Warn if code-owners plugin configuration is added in project.config
The configuration of the code-owners plugin is read from
* gerrit.config (section 'plugin.code-owners')
* code-owners.config in refs/meta/config (mostly section 'codeOwners')
Sometimes it happens that users wrongly add code-owners plugin
configuration in project.config and then don't understand why the
configuration doesn't work.
Print out a warning if code-owners plugin configuration is added in
project.config so that users get to know that this is the wrong place to
configure the code-owners plugin.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ice385139c41cae421297f12895cdd1a6efff8b93
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
index 651ce61..df8003f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigValidator.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.backend.config;
+import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS;
+
import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.RefNames;
@@ -77,30 +79,40 @@
@Override
public ImmutableList<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
- String fileName = pluginName + ".config";
+ if (!receiveEvent.refName.equals(RefNames.REFS_CONFIG)) {
+ // The code-owners.config file is stored in refs/meta/config, if refs/meta/config was not
+ // modified we do not need to do any validation and can return early.
+ return ImmutableList.of();
+ }
+ ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder =
+ ImmutableList.builder();
+ validationMessageBuilder.addAll(
+ getWarningsForIgnoredCodeOwnerConfigurationInProjectConfig(receiveEvent));
+ String codeOwnersConfigFileName = pluginName + ".config";
try {
- if (!receiveEvent.refName.equals(RefNames.REFS_CONFIG)
- || !isFileChanged(receiveEvent, fileName)) {
+ if (!isFileChanged(receiveEvent, codeOwnersConfigFileName)) {
// the code-owners.config file in refs/meta/config was not modified, hence we do not need to
// validate it
- return ImmutableList.of();
+ return validationMessageBuilder.build();
}
ProjectState projectState = getProjectState(receiveEvent);
- ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, fileName);
- ImmutableList<CommitValidationMessage> validationMessages =
- validateConfig(projectState, fileName, cfg.getConfig());
+ ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, codeOwnersConfigFileName);
+ validationMessageBuilder.addAll(
+ validateConfig(projectState, codeOwnersConfigFileName, cfg.getConfig()));
+
+ ImmutableList<CommitValidationMessage> validationMessages = validationMessageBuilder.build();
if (!validationMessages.isEmpty()) {
throw new CommitValidationException(
- exceptionMessage(fileName, cfg.getRevision()), validationMessages);
+ exceptionMessage(codeOwnersConfigFileName, cfg.getRevision()), validationMessages);
}
return ImmutableList.of();
} catch (IOException | DiffNotAvailableException | ConfigInvalidException e) {
String errorMessage =
String.format(
"failed to validate file %s for revision %s in ref %s of project %s",
- fileName,
+ codeOwnersConfigFileName,
receiveEvent.commit.getName(),
RefNames.REFS_CONFIG,
receiveEvent.project.getNameKey());
@@ -109,6 +121,53 @@
}
}
+ private ImmutableList<CommitValidationMessage>
+ getWarningsForIgnoredCodeOwnerConfigurationInProjectConfig(CommitReceivedEvent receiveEvent) {
+ try {
+ if (!isFileChanged(receiveEvent, ProjectConfig.PROJECT_CONFIG)) {
+ return ImmutableList.of();
+ }
+
+ ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder =
+ ImmutableList.builder();
+ ProjectLevelConfig.Bare cfg = loadConfig(receiveEvent, ProjectConfig.PROJECT_CONFIG);
+
+ if (cfg.getConfig().getSubsections("plugin").contains(pluginName)) {
+ // The plugin.code-owners section is only read from gerrit.config, but not from
+ // project.config. Warn that this configuration is ignored and has no effect.
+ validationMessageBuilder.add(
+ new CommitValidationMessage(
+ String.format(
+ "Section 'plugin.code-owners' in %s is ignored and has no effect."
+ + " The configuration for the %s plugin must be done in %s.config.",
+ ProjectConfig.PROJECT_CONFIG, pluginName, pluginName),
+ ValidationMessage.Type.HINT));
+ }
+
+ if (cfg.getConfig().getSections().contains(SECTION_CODE_OWNERS)) {
+ // The codeOwners section is only read from code-owners.config, but not from
+ // project.config. Warn that this configuration is ignored and has no effect.
+ validationMessageBuilder.add(
+ new CommitValidationMessage(
+ String.format(
+ "Section 'codeOwners' in %s is ignored and has no effect."
+ + " The configuration for the %s plugin must be done in %s.config.",
+ ProjectConfig.PROJECT_CONFIG, pluginName, pluginName),
+ ValidationMessage.Type.HINT));
+ }
+
+ return validationMessageBuilder.build();
+ } catch (IOException | DiffNotAvailableException | CommitValidationException e) {
+ logger.atSevere().withCause(e).log(
+ "failed to inspect file %s for revision %s in ref %s of project %s",
+ ProjectConfig.PROJECT_CONFIG,
+ receiveEvent.commit.getName(),
+ RefNames.REFS_CONFIG,
+ receiveEvent.project.getNameKey());
+ return ImmutableList.of();
+ }
+ }
+
private ProjectState getProjectState(CommitReceivedEvent receiveEvent)
throws IOException, ConfigInvalidException {
ProjectConfig projectConfig = projectConfigFactory.create(receiveEvent.project.getNameKey());
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 f7f94be..b2a6630 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -803,6 +803,63 @@
ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
}
+ @Test
+ public void warnIfCodeOwnersConfigurationIsDoneInProjectConfig_configWithPluginCodeOwnersSection()
+ throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setEnum(
+ /* section= */ "plugin",
+ /* subsection= */ "code-owners",
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertMessage(
+ String.format(
+ "hint: commit %s: Section 'plugin.code-owners' in project.config is ignored and has no"
+ + " effect. The configuration for the code-owners plugin must be done in"
+ + " code-owners.config.",
+ ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
+ }
+
+ @Test
+ public void warnIfCodeOwnersConfigurationIsDoneInProjectConfig_configWithCodeOwnersSection()
+ throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setEnum(
+ CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ GeneralConfig.KEY_FALLBACK_CODE_OWNERS,
+ FallbackCodeOwners.ALL_USERS);
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertMessage(
+ String.format(
+ "hint: commit %s: Section 'codeOwners' in project.config is ignored and has no effect."
+ + " The configuration for the code-owners plugin must be done in code-owners.config.",
+ ObjectIds.abbreviateName(r.getCommit(), testRepo.getRevWalk().getObjectReader())));
+ }
+
+ @Test
+ public void noWarningIfProjectConfigIsUpdatedWithoutCodeOwnerSettings() throws Exception {
+ fetchRefsMetaConfig();
+ Config cfg = new Config();
+ cfg.setString(
+ /* section= */ "foo", /* subsection= */ null, /* name= */ "bar", /* value= */ "baz");
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change", "project.config", cfg.toText());
+ PushOneCommit.Result r = push.to("refs/for/" + RefNames.REFS_CONFIG);
+ r.assertOkStatus();
+ r.assertNotMessage(
+ "The configuration for the code-owners plugin must be done in code-owners.config.");
+ }
+
private void fetchRefsMetaConfig() throws Exception {
fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
testRepo.reset(RefNames.REFS_CONFIG);
