From 43d552e8500e483d2f312839b7e310a54ed42af8 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Wed, 8 Mar 2023 12:36:46 +0100 Subject: [PATCH 1/2] [github_pr_destination] Don't require context reference if `pr_branch` is set This will allow copybara to succeed for the following origin: ``` git.origin( url = "https://github.com/google/copybara.git", ref = "b0f6c6bbb5828c95b2b1409b4e491865d969f679", ) ``` --- .../copybara/git/GitHubPrDestination.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/java/com/google/copybara/git/GitHubPrDestination.java b/java/com/google/copybara/git/GitHubPrDestination.java index 1fac88bee..acc592ade 100644 --- a/java/com/google/copybara/git/GitHubPrDestination.java +++ b/java/com/google/copybara/git/GitHubPrDestination.java @@ -337,14 +337,18 @@ private String getPullRequestBranchName( return gitHubDestinationOptions.destinationPrBranch; } 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 branchNameFromUser = prBranch; + if (branchNameFromUser == null) { + // 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); + branchNameFromUser = getCustomBranchName(contextReference); + } String branchName = branchNameFromUser != null ? branchNameFromUser From 1e8dc787acf9f1220c8e5e07252883d3500fc168 Mon Sep 17 00:00:00 2001 From: Corbin McNeely-Smith Date: Sat, 11 Mar 2023 16:53:34 -0600 Subject: [PATCH 2/2] Only fail on missing context reference when it is required. * This appears to be a reasonable compromise given that there are many ways to create a Revision without context reference. * Add null handling for Revision in pr branch name resolution (as it's marked with Nullable, however *that* works.) * Fail when labels other than CONTEXT_REFERENCE are used in the pr name. (bit of a bug -- previously, for any interpolated label would just reprint the label. Since no tests failed when fixed, it appears to be unexpected behaviour.) * Side note: these files seem to have missed the goggle-java-format boat. :[ --- .../copybara/git/GitHubPrDestination.java | 82 ++++++++++++------- .../copybara/git/GitHubPrDestinationTest.java | 41 ++++++++-- 2 files changed, 87 insertions(+), 36 deletions(-) diff --git a/java/com/google/copybara/git/GitHubPrDestination.java b/java/com/google/copybara/git/GitHubPrDestination.java index acc592ade..96de3bcca 100644 --- a/java/com/google/copybara/git/GitHubPrDestination.java +++ b/java/com/google/copybara/git/GitHubPrDestination.java @@ -333,48 +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; - } - String contextReference = changeRevision.contextReference(); - - String branchNameFromUser = prBranch; - if (branchNameFromUser == null) { - // 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); - branchNameFromUser = getCustomBranchName(contextReference); + // 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 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; @@ -404,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");