diff --git a/java/com/google/copybara/git/GitHubPrDestination.java b/java/com/google/copybara/git/GitHubPrDestination.java index 1fac88bee..96de3bcca 100644 --- a/java/com/google/copybara/git/GitHubPrDestination.java +++ b/java/com/google/copybara/git/GitHubPrDestination.java @@ -333,44 +333,68 @@ public Iterable getIntegrates() { private String getPullRequestBranchName( @Nullable Revision changeRevision, String workflowName, String workflowIdentityUser) throws ValidationException { + String resolvedPrBranchName; if (!Strings.isNullOrEmpty(gitHubDestinationOptions.destinationPrBranch)) { - return gitHubDestinationOptions.destinationPrBranch; + // command line provided value has highest priority + resolvedPrBranchName = gitHubDestinationOptions.destinationPrBranch; + } else if (prBranch != null) { + // next, the starlark provided value, with resolved labels. + resolvedPrBranchName = resolvePrBranchLabels(prBranch, changeRevision); + } else { + // finally, attempt a sane branch name from the workflow + // change revision and context are required to compute a pr branch name. + if (changeRevision == null || changeRevision.contextReference() == null) { + throw new ValidationException(MISSING_CONTEXT_REFERENCE_MESSAGE); + } + resolvedPrBranchName = + Identity.computeIdentity( + "OriginGroupIdentity", + changeRevision.contextReference(), + workflowName, + mainConfigFile.getIdentifier(), + workflowIdentityUser); } - String contextReference = changeRevision.contextReference(); - // We could do more magic here with the change identity. But this is already complex so we - // require a group identity either provided by the origin or the workflow (Will be implemented - // later. - checkCondition(contextReference != null, - "git.github_pr_destination is incompatible with the current origin. Origin has to be" - + " able to provide the contextReference or use '%s' flag", - GitHubDestinationOptions.GITHUB_DESTINATION_PR_BRANCH); - String branchNameFromUser = getCustomBranchName(contextReference); - String branchName = - branchNameFromUser != null - ? branchNameFromUser - : Identity.computeIdentity( - "OriginGroupIdentity", - contextReference, - workflowName, - mainConfigFile.getIdentifier(), - workflowIdentityUser); - return GitHubUtil.getValidBranchName(branchName); + return GitHubUtil.getValidBranchName(resolvedPrBranchName); } - @Nullable - private String getCustomBranchName(String contextReference) throws ValidationException { - if (prBranch == null) { - return null; - } + private static String resolvePrBranchLabels(String prBranch, Revision changeRevision) + throws ValidationException { try { return new LabelTemplate(prBranch) - .resolve(e -> e.equals("CONTEXT_REFERENCE") ? contextReference : prBranch); + .resolve( + e -> { + // no labels can be resolved without a revision. + if (changeRevision == null) { + return null; + } + // associatedLabels may contain CONTEXT_REFERENCE + if (e.equals("CONTEXT_REFERENCE")) { + return changeRevision.contextReference(); + } + // no other labels are supported, according to the docs. + return null; + }); } catch (LabelNotFoundException e) { + if ("CONTEXT_REFERENCE".equals(e.getLabel())) { + // contextReference was needed to resolve the branch. + // This lazily validates the context reference, allowing support for the + // case where the Revision is missing or lacks context, but the workflow + // does not ask for it. + // See GitRepository.fetchSingleRefWithTags for cases where this is true. + throw new ValidationException(MISSING_CONTEXT_REFERENCE_MESSAGE); + } + throw new ValidationException( "Cannot find some labels in the GitHub PR branch name field: " + e.getMessage(), e); } } + @VisibleForTesting + static final String MISSING_CONTEXT_REFERENCE_MESSAGE = String.format( + "git.github_pr_destination is incompatible with the current origin." + + "Origin has to be able to provide the contextReference or use '%s' flag", + GitHubDestinationOptions.GITHUB_DESTINATION_PR_BRANCH); + @Override public String getLabelNameWhenOrigin() { return GitRepository.GIT_ORIGIN_REV_ID; @@ -400,4 +424,4 @@ private static class GitHubWriterState extends WriterState { } return resolvedDestinationRef; } -} \ No newline at end of file +} diff --git a/javatests/com/google/copybara/git/GitHubPrDestinationTest.java b/javatests/com/google/copybara/git/GitHubPrDestinationTest.java index f81172179..7ae1f5d7b 100644 --- a/javatests/com/google/copybara/git/GitHubPrDestinationTest.java +++ b/javatests/com/google/copybara/git/GitHubPrDestinationTest.java @@ -128,10 +128,7 @@ public void testWrite_noContextReference() throws ValidationException { assertThrows(ValidationException.class, () -> d.newWriter(writerContext)); assertThat(thrown) .hasMessageThat() - .contains( - "git.github_pr_destination is incompatible with the current origin." - + " Origin has to be able to provide the contextReference or use" - + " '--github-destination-pr-branch' flag"); + .contains(GitHubPrDestination.MISSING_CONTEXT_REFERENCE_MESSAGE); } @Test @@ -301,8 +298,42 @@ public void testWrite_destinationPrBranchFlag() } @Test - public void testTrimMessageForPrTitle() + public void testWrite_destinationPrBranchHasMissingLabel() { + assertThat( + assertThrows( + ValidationException.class, + () -> testBranchNameFromUser( + "copybara/import_foo_${MISSING}", + null, + null))).hasMessageThat().contains("MISSING"); + } + + @Test + public void testWrite_destinationPrBranchNoContextReferenceRequired() throws ValidationException, IOException, RepoException { + testBranchNameFromUser( + "copybara/import_foo", + "copybara/import_foo", + null + ); + } + + @Test + public void testWrite_destinationPrBranchContextExplicitlyReferenceRequired() { + assertThat( + assertThrows( + ValidationException.class, + () -> + testBranchNameFromUser( + "copybara/import_foo_${CONTEXT_REFERENCE}", + null, + null))) + .hasMessageThat() + .contains(GitHubPrDestination.MISSING_CONTEXT_REFERENCE_MESSAGE); + } + + @Test + public void testTrimMessageForPrTitle() throws ValidationException, IOException, RepoException { options.githubDestination.destinationPrBranch = "feature"; mockNoPullRequestsGet("feature");