Migrate label fns to SRs: Adapt logic for 'branch' label attribute If the label definition contained a 'branch' field, the label was only applicable and voteable on these specific branches (see [1]). Adapting the label fns to SRs migrator logic to handle this case: add an OR'ed `applicableIf` expression to the SR of all branches occurring in the label def. This should also work with branches with regex ref pattern. [1] https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_branch Google-Bug-Id: b/264859003 Release-Notes: skip Change-Id: I8ddd6111079744e021d069df09e5e8c51752aa0e
diff --git a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java index 491b5cd..f3c741f 100644 --- a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java +++ b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
@@ -37,6 +37,7 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; import javax.inject.Inject; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @@ -216,12 +217,19 @@ Arrays.asList( cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_VALUE))) .build(); + ImmutableList<String> refPatterns = + ImmutableList.<String>builder() + .addAll( + Arrays.asList( + cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_BRANCH))) + .build(); LabelAttributes attributes = LabelAttributes.create( function == null ? "MaxWithBlock" : function, canOverride, ignoreSelfApproval, - values); + values, + refPatterns); labelTypes.put(labelName, attributes); } return labelTypes; @@ -320,6 +328,15 @@ default: break; } + if (!attributes.refPatterns().isEmpty()) { + builder.setApplicabilityExpression( + SubmitRequirementExpression.of( + String.join( + " OR ", + attributes.refPatterns().stream() + .map(b -> "branch:\\\"" + b + "\\\"") + .collect(Collectors.toList())))); + } return builder.build(); } @@ -435,13 +452,16 @@ abstract ImmutableList<String> values(); + abstract ImmutableList<String> refPatterns(); + static LabelAttributes create( String function, boolean canOverride, boolean ignoreSelfApproval, - ImmutableList<String> values) { + ImmutableList<String> values, + ImmutableList<String> refPatterns) { return new AutoValue_MigrateLabelFunctionsToSubmitRequirement_LabelAttributes( - function, canOverride, ignoreSelfApproval, values); + function, canOverride, ignoreSelfApproval, values, refPatterns); } } }
diff --git a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java index 74bfe0f..9d37497 100644 --- a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java +++ b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
@@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -260,6 +261,96 @@ } @Test + public void migrateBlockingLabel_withBranchAttribute() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/master")); + + assertNonExistentSr(/* srName = */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withMultipleBranchAttributes() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/master", "refs/heads/develop")); + + assertNonExistentSr(/* srName = */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" " + + "OR branch:\\\"refs/heads/develop\\\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withRegexBranchAttribute() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("^refs/heads/main-.*")); + + assertNonExistentSr(/* srName = */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\\\"^refs/heads/main-.*\\\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withRegexAndNonRegexBranchAttributes() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/master", "^refs/heads/main-.*")); + + assertNonExistentSr(/* srName = */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" " + + "OR branch:\\\"^refs/heads/main-.*\\\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test public void migrationIsIdempotent() throws Exception { String oldRefsConfigId; try (Repository repo = repoManager.openRepository(project)) { @@ -381,6 +472,21 @@ gApi.projects().name(project.get()).label(labelName).create(input); } + private void createLabelWithBranch( + String labelName, + String function, + boolean ignoreSelfApproval, + ImmutableList<String> refPatterns) + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.name = labelName; + input.function = function; + input.ignoreSelfApproval = ignoreSelfApproval; + input.values = ImmutableMap.of("+1", "Looks Good", " 0", "Don't Know", "-1", "Looks Bad"); + input.branches = refPatterns; + gApi.projects().name(project.get()).label(labelName).create(input); + } + @CanIgnoreReturnValue private SubmitRequirementApi createSubmitRequirement( String name, String submitExpression, boolean canOverride) throws Exception {