[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:masterfrom
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",
)
```
Yannic
force-pushed
the
yannic-pr-branch-destination
branch
from
f52ff4e to
6ba12d2
Compare
petemounce
left a comment
petemounce
left a comment
There was a problem hiding this comment.
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)
| GitHubDestinationOptions.GITHUB_DESTINATION_PR_BRANCH); | ||
| String branchNameFromUser = getCustomBranchName(contextReference); | ||
|
|
||
| String branchNameFromUser = prBranch; |
There was a problem hiding this comment.
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!!
4 participants
This will allow copybara to succeed for the following origin: