-
Notifications
You must be signed in to change notification settings - Fork 304
[github_pr_destination] Don't require context reference if pr_branch is set#226
[github_pr_destination] Don't require context reference if pr_branch is set #226Yannic wants to merge 1 commit intogoogle:master from
pr_branch is set #226Conversation
...` is set This will allow copybara to succeed for the following origin: ``` git.origin( url = "https://github.com/google/copybara.git", ref = "b0f6c6bbb5828c95b2b1409b4e491865d969f679", ) ```
f52ff4e to
6ba12d2
Compare
@petemounce
petemounce
left a comment
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.
Thanks!
(Context; we're using copybara to automate vendoring of source-code into a monorepo, but don't always want to continuously track & sync upstream branches. Sometimes we just want to pin to specific revisions and deliberately change those as/when we want to take updates from upstreams. This is then conveniently wrapped up in a CI job that can be triggered when the choice to sync to a different revision happens and is reviewed).
(I don't know if my approve counts for anything, but there it is)
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 think the code is a bit confusing after this change.
When prBranch == null, we get branchNameFromUser == null (now the only reason to enter the if statement),
But then we call:
if (prBranch == null) {
return null;
}
...
effectively returning null.
I would refactor this code to have something that makes sense overall.
Also, could you add a tests in GitHubPrDestinationTest?
Thanks for the contribution!!
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.
Addressed changes in #229
This will allow copybara to succeed for the following origin: