Always fill out PR title after conflict resolution - #20
Conversation
Sorry, something went wrong.
Before this change, cherry-picker used to loose the backported commit message and didn't pass anything to the `push_to_remote()` method which resulted in sending an API request to GitHub that had an empty title upon resume. This caused GitHub respond with an error. This code path was only problematic for people having a GitHub token in their env and therefore enjoying the benefits of the PR autocreation. After this change, cherry-picker picks up the commit message correctly and extracts the proper PR title and body out of it.
|
@sivel this should fix your problem ☝️ |
Sorry, something went wrong.
|
Please ignore the nightly job, its failure is unrelated to this change. It happens because pytest turns all warnings into errors. |
Sorry, something went wrong.
|
I've confirmed this fixes my issue. Instead of: 422
{"message":"Validation Failed","errors":[{"resource":"Issue","code":"missing_field","field":"title"}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}
The PR is properly created. |
Sorry, something went wrong.
| return updated_commit_message | ||
|
|
||
| def push_to_remote(self, base_branch, head_branch, commit_message=""): | ||
| def push_to_remote(self, base_branch, head_branch, commit_message): |
There was a problem hiding this comment.
Why is removing the default necessary?
Sorry, something went wrong.
There was a problem hiding this comment.
@webknjaz can we keep the original behavior and let the default commit message?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't remember, it was over a year ago. But I think I wanted to prevent situations when a caller would accidentally leave it empty. Why would one want this behavior to be implicit? If a caller wants an empty title, they should state that explicitly instead of relying on the magical empty value. Besides, it's passed to create_gh_pr() without changes, which already enforces the same.
Sorry, something went wrong.
There was a problem hiding this comment.
FTR, I've checked that the only known consumer does not call this method directly: https://github.com/python/miss-islington/blob/23841c0b44b84981d0db055e7eb0c61a532b5aa4/miss_islington/tasks.py#L128. And there's probably no other public API user in the wild.
Having no default here is intended to force the callers to always compute the PR title as opposed to allowing it to be empty when forgotten.
Sorry, something went wrong.
|
Safe to say this ain't happening. Sorry to have wasted your time, Sviatoslav. |
Sorry, something went wrong.
|
It's okay, Łukasz. Ping me if that changes in the future and I'll gladly take another look! FWIW, some of my colleagues at Ansible were hitting this corner case, hence the PR. I usually don't have a token in env vars.. |
Sorry, something went wrong.
|
Let's try again. |
Sorry, something went wrong.
Was this just routine issue cleanup, or is the fix not worth doing? |
Sorry, something went wrong.
|
I've rebased this locally and will push everything once I add some integration tests. |
Sorry, something went wrong.
Before this change, cherry-picker used to loose the backported commit
message and didn't pass anything to the push_to_remote() method which
resulted in sending an API request to GitHub that had an empty title
upon resume. This caused GitHub respond with an error.
This code path was only problematic for people having a GitHub token in
their env and therefore enjoying the benefits of the PR autocreation.
After this change, cherry-picker picks up the commit message correctly
and extracts the proper PR title and body out of it.