Allow to define and configure override label in the same commit When a code-owners.config file is pushed we validate the override configuration in it. This includes checking if the configured label exists for the project. If the label didn't exist yet, but was configured in the commit that is being pushed, we didn't take it into account and wrongly rejected the commit, saying that the label doesn't exist (because we read the labels from the head revision of refs/meta/config and not from the commit that was being pushed). This is fixed now, by reading the available labels from the commit that is being pushed. Change-Id: Ifecaca554605c123c62c347b54cb873f7cc7e09d Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java index 2a29766..8f20fc1 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java +++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigValidator.java
@@ -14,8 +14,6 @@ package com.google.gerrit.plugins.codeowners.config; -import static com.google.gerrit.server.project.ProjectCache.illegalState; - import com.google.common.collect.ImmutableList; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Project; @@ -30,7 +28,7 @@ import com.google.gerrit.server.git.validators.CommitValidationMessage; import com.google.gerrit.server.git.validators.ValidationMessage; import com.google.gerrit.server.patch.PatchListNotAvailableException; -import com.google.gerrit.server.project.ProjectCache; +import com.google.gerrit.server.project.ProjectConfig; import com.google.gerrit.server.project.ProjectLevelConfig; import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; @@ -41,6 +39,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; /** Validates modifications to the {@code code-owners.config} file in {@code refs/meta/config}. */ @Singleton @@ -48,8 +47,9 @@ private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final String pluginName; - private final ProjectCache projectCache; private final GitRepositoryManager repoManager; + private final ProjectConfig.Factory projectConfigFactory; + private final ProjectState.Factory projectStateFactory; private final ChangedFiles changedFiles; private final GeneralConfig generalConfig; private final StatusConfig statusConfig; @@ -60,8 +60,9 @@ @Inject CodeOwnersPluginConfigValidator( @PluginName String pluginName, - ProjectCache projectCache, GitRepositoryManager repoManager, + ProjectConfig.Factory projectConfigFactory, + ProjectState.Factory projectStateFactory, ChangedFiles changedFiles, GeneralConfig generalConfig, StatusConfig statusConfig, @@ -69,8 +70,9 @@ RequiredApprovalConfig requiredApprovalConfig, OverrideApprovalConfig overrideApprovalConfig) { this.pluginName = pluginName; - this.projectCache = projectCache; this.repoManager = repoManager; + this.projectConfigFactory = projectConfigFactory; + this.projectStateFactory = projectStateFactory; this.changedFiles = changedFiles; this.generalConfig = generalConfig; this.statusConfig = statusConfig; @@ -93,11 +95,11 @@ return ImmutableList.of(); } - ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project)); + ProjectState projectState = getProjectState(project, receiveEvent.commit); ProjectLevelConfig.Bare cfg = loadConfig(project, fileName, receiveEvent.commit); validateConfig(projectState, fileName, cfg); return ImmutableList.of(); - } catch (IOException | PatchListNotAvailableException e) { + } catch (IOException | ConfigInvalidException | PatchListNotAvailableException e) { String errorMessage = String.format( "failed to validate file %s for revision %s in ref %s of project %s", @@ -107,6 +109,15 @@ } } + private ProjectState getProjectState(Project.NameKey projectName, RevCommit commit) + throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(projectName)) { + ProjectConfig projectConfig = projectConfigFactory.create(projectName); + projectConfig.load(repo, commit); + return projectStateFactory.create(projectConfig.getCacheable()); + } + } + /** * Whether the given file was changed in the given revision. *
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java index 93504a7..1ea4183 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -37,8 +37,10 @@ import com.google.gerrit.plugins.codeowners.config.StatusConfig; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RemoteRefUpdate.Status; +import org.eclipse.jgit.util.RawParseUtils; import org.junit.Before; import org.junit.Test; @@ -369,6 +371,52 @@ } @Test + public void defineAndConfigureOverrideLabelInSameCommit() throws Exception { + fetchRefsMetaConfig(); + + RevCommit head = getHead(testRepo.getRepository(), RefNames.REFS_CONFIG); + RevObject blob = testRepo.get(head.getTree(), "project.config"); + byte[] data = testRepo.getRepository().open(blob).getCachedBytes(Integer.MAX_VALUE); + String projectConfigText = RawParseUtils.decode(data); + + Config projectConfig = new Config(); + projectConfig.fromText(projectConfigText); + String labelName = "Owners-Override"; + projectConfig.setString("label", labelName, "function", "NoOp"); + projectConfig.setStringList( + "label", labelName, "value", ImmutableList.of("0 Not Override", "+1 Override")); + + Config codeOwnersConfig = new Config(); + codeOwnersConfig.setString( + CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS, + /* subsection= */ null, + OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL, + "Owners-Override+1"); + + RevCommit commit = + testRepo.update( + RefNames.REFS_CONFIG, + testRepo + .commit() + .parent(head) + .message("Add test code owner config") + .author(admin.newIdent()) + .committer(admin.newIdent()) + .add("code-owners.config", codeOwnersConfig.toText()) + .add("project.config", projectConfig.toText())); + + testRepo.reset(commit); + + PushResult r = pushRefsMetaConfig(); + assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK); + ImmutableSet<RequiredApproval> overrideApproval = + codeOwnersPluginConfiguration.getOverrideApproval(project); + assertThat(overrideApproval).hasSize(1); + assertThat(overrideApproval).element(0).hasLabelNameThat().isEqualTo("Owners-Override"); + assertThat(overrideApproval).element(0).hasValueThat().isEqualTo(1); + } + + @Test public void configureMergeCommitStrategy() throws Exception { fetchRefsMetaConfig();