Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always fill out PR title after conflict resolution #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webknjaz
Copy link
Contributor

@webknjaz webknjaz commented Feb 5, 2021

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.

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.
@webknjaz
Copy link
Contributor Author

webknjaz commented Feb 5, 2021

@sivel this should fix your problem ☝️

@webknjaz
Copy link
Contributor Author

webknjaz commented Feb 5, 2021

Please ignore the nightly job, its failure is unrelated to this change. It happens because pytest turns all warnings into errors.

@sivel
Copy link

sivel commented Feb 5, 2021

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.

@jaraco jaraco closed this Jun 3, 2021
@jaraco jaraco reopened this Jun 3, 2021
@@ -277,7 +277,7 @@ def amend_commit_message(self, cherry_pick_branch):
click.echo(cpe.output)
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is removing the default necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webknjaz can we keep the original behavior and let the default commit message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants