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
Add support for merge-upstream Repository action #2066
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking like an excellent start, just need to sort out the test failures.
@@ -3,19 +3,20 @@ GET | |||
api.github.com | |||
None | |||
/user | |||
{'Authorization': 'Basic login_and_password_removed', 'User-Agent': 'PyGithub/Python'} | |||
{'Authorization': 'token private_token_removed', 'User-Agent': 'PyGithub/Python'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is one reason why CI fails -- sadly it's a sharp edge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was ... confused. I think I've fixed it, though? We'll see when the tests finish again, I guess.
self.assertTrue(self.repo.merge_upstream("master")) | ||
|
||
def testMergeUpstreamFailure(self): | ||
self.assertFalse(self.repo.merge_upstream("doesNotExist")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if this actually throws an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only two error codes GitHub gives you on failure are either "409 Conflict" if you have a merge conflict or "422 Unprocessable Entity" if "some other reason" prevented the merge. I briefly thought of raising an exception on the 422, and returning False
for 409, but I wasn't totally sure everything related to a 422 was something truly exceptional. But it sort of makes sense, I guess? Maybe if I use the response message as the exception text?
It's now possible to trigger a merge from upstream on a branch directly via the API rather than trying to compare commit hashes and editing refs. This commit adds support for the new action.
0d41822
to
46a589e
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@s-t-e-v-e-n-k I have (I believe) fixed the test failures, but I can't run them myself. I also left a question for you on a reply to a comment. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@s-t-e-v-e-n-k Pinging again to keep it on your list and save it from the stale bot. |
It's now possible to trigger a merge from upstream on a branch directly via the API rather than trying to compare commit hashes and editing refs. I've added support for the new action.
Note: as of creating this pull request, the new action is beta. It may change. I'm creating this pull request now to track it. It does, however, currently work.