From 0a2769d4f2c86a6b7f6104f240a75650aa34caaa Mon Sep 17 00:00:00 2001 From: Pierre Beitz Date: Sun, 17 Nov 2019 14:56:32 +0100 Subject: [PATCH] Fix code review --- pom.xml | 18 +++- .../plugins/audit_trail/AuditTrailFilter.java | 11 ++- .../plugins/audit_trail/AuditTrailPlugin.java | 14 ++- .../audit_trail/ConfigurationAsCodeTest.java | 94 +++++++++++-------- .../hudson/plugins/audit_trail/expected.yml | 18 ++++ 5 files changed, 102 insertions(+), 53 deletions(-) create mode 100644 src/test/resources/hudson/plugins/audit_trail/expected.yml diff --git a/pom.xml b/pom.xml index 8027c43..1b23014 100644 --- a/pom.xml +++ b/pom.xml @@ -14,9 +14,8 @@ Audit Trail 2.6-SNAPSHOT - 2.60.3 + 2.138.1 8 - 1.23 http://wiki.jenkins-ci.org/display/JENKINS/Audit+Trail+Plugin @@ -43,13 +42,11 @@ io.jenkins configuration-as-code - ${configuration-as-code.version} test io.jenkins configuration-as-code - ${configuration-as-code.version} tests test @@ -91,4 +88,17 @@ https://repo.jenkins-ci.org/public/ + + + + + io.jenkins.tools.bom + bom-2.138.x + 3 + import + pom + + + + diff --git a/src/main/java/hudson/plugins/audit_trail/AuditTrailFilter.java b/src/main/java/hudson/plugins/audit_trail/AuditTrailFilter.java index 17b1b09..5fcccfc 100644 --- a/src/main/java/hudson/plugins/audit_trail/AuditTrailFilter.java +++ b/src/main/java/hudson/plugins/audit_trail/AuditTrailFilter.java @@ -123,10 +123,15 @@ public static void init() throws ServletException { } private void onRequest(String uri, String extra, String username) { - if (configuration.isStarted()) { - for (AuditLogger logger : configuration.getLoggers()) { - logger.log(uri + extra + " by " + username); + if (configuration != null) { + if (configuration.isStarted()) { + for (AuditLogger logger : configuration.getLoggers()) { + logger.log(uri + extra + " by " + username); + } + } else { + LOGGER.warning("Plugin configuration not properly injected, please report an issue to the Audit Trail Plugin"); } } + } } diff --git a/src/main/java/hudson/plugins/audit_trail/AuditTrailPlugin.java b/src/main/java/hudson/plugins/audit_trail/AuditTrailPlugin.java index d0139e7..f0421f3 100644 --- a/src/main/java/hudson/plugins/audit_trail/AuditTrailPlugin.java +++ b/src/main/java/hudson/plugins/audit_trail/AuditTrailPlugin.java @@ -23,7 +23,6 @@ */ package hudson.plugins.audit_trail; -import hudson.BulkChange; import hudson.DescriptorExtensionList; import hudson.Extension; @@ -34,6 +33,8 @@ import jenkins.model.GlobalConfiguration; import net.sf.json.JSONObject; import org.jenkinsci.Symbol; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.DoNotUse; import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.StaplerRequest; @@ -80,16 +81,11 @@ public AuditTrailPlugin() { } @Override - public boolean configure(StaplerRequest req, JSONObject formData) throws FormException { + public boolean configure(StaplerRequest req, JSONObject formData) { // readResolve makes sure loggers is initialized, so it should never be null. // TODO this should probably be moved somewhere else loggers.forEach(AuditLogger::cleanUp); - try (BulkChange bc = new BulkChange(this)) { - req.bindJSON(this, formData); - bc.commit(); - } catch (IOException e) { - throw new FormException("Failed to apply configuration", e, null); - } + req.bindJSON(this, formData); applySettings(); return true; } @@ -141,6 +137,7 @@ boolean isStarted() { /** * @deprecated as of 2.6 **/ + @Restricted(DoNotUse.class) @Deprecated public void onFinalized(Run run) { LOGGER.warning("AuditTrailPlugin#onFinalized does nothing anymore, please update your script"); @@ -149,6 +146,7 @@ public void onFinalized(Run run) { /** * @deprecated as of 2.6 **/ + @Restricted(DoNotUse.class) @Deprecated public void onFinalized(AbstractBuild build) { LOGGER.warning("AuditTrailPlugin#onFinalized does nothing anymore, please update your script"); diff --git a/src/test/java/hudson/plugins/audit_trail/ConfigurationAsCodeTest.java b/src/test/java/hudson/plugins/audit_trail/ConfigurationAsCodeTest.java index cc5062e..9c89b9e 100644 --- a/src/test/java/hudson/plugins/audit_trail/ConfigurationAsCodeTest.java +++ b/src/test/java/hudson/plugins/audit_trail/ConfigurationAsCodeTest.java @@ -1,14 +1,18 @@ package hudson.plugins.audit_trail; import hudson.ExtensionList; +import io.jenkins.plugins.casc.ConfigurationContext; +import io.jenkins.plugins.casc.ConfiguratorRegistry; import io.jenkins.plugins.casc.misc.ConfiguredWithCode; import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule; +import io.jenkins.plugins.casc.model.CNode; import org.junit.ClassRule; import org.junit.Test; import org.jvnet.hudson.test.Issue; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static io.jenkins.plugins.casc.misc.Util.*; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.*; /** * Created by Pierre Beitz @@ -16,40 +20,54 @@ */ public class ConfigurationAsCodeTest { - @ClassRule - @ConfiguredWithCode("jcasc.yml") - public static JenkinsConfiguredWithCodeRule r = new JenkinsConfiguredWithCodeRule(); - - @Issue("JENKINS-57232") - @Test - public void should_support_configuration_as_code() { - ExtensionList extensionList = r.jenkins.getExtensionList(AuditTrailPlugin.class); - AuditTrailPlugin plugin = extensionList.get(0); - assertEquals(".*/(?:configSubmit|doUninstall|doDelete|postBuildResult|enable|disable|cancelQueue|stop|toggleLogKeep|doWipeOutWorkspace|createItem|createView|toggleOffline|cancelQuietDown|quietDown|restart|exit|safeExit)", plugin.getPattern()); - assertTrue(plugin.getLogBuildCause()); - assertEquals(3, plugin.getLoggers().size()); - - //first logger - AuditLogger logger = plugin.getLoggers().get(0); - assertTrue(logger instanceof ConsoleAuditLogger); - assertEquals("test", ((ConsoleAuditLogger) logger).getLogPrefix()); - assertEquals(ConsoleAuditLogger.Output.STD_OUT, ((ConsoleAuditLogger) logger).getOutput()); - assertEquals("yyyy-MM-dd HH:mm:ss:SSS", ((ConsoleAuditLogger)logger).getDateFormat()); - - //second logger - logger = plugin.getLoggers().get(1); - assertTrue(logger instanceof LogFileAuditLogger); - assertEquals(10, ((LogFileAuditLogger)logger).getCount()); - assertEquals(666, ((LogFileAuditLogger)logger).getLimit()); - assertEquals("/log/location", ((LogFileAuditLogger)logger).getLog()); - - //third logger - logger = plugin.getLoggers().get(2); - assertEquals("jenkins", ((SyslogAuditLogger)logger).getAppName()); - assertEquals("DAEMON", ((SyslogAuditLogger)logger).getFacility()); - assertEquals("RFC_5424", ((SyslogAuditLogger)logger).getMessageFormat()); - assertEquals("hostname", ((SyslogAuditLogger)logger).getMessageHostname()); - assertEquals("syslog-server", ((SyslogAuditLogger)logger).getSyslogServerHostname()); - assertEquals(514, ((SyslogAuditLogger)logger).getSyslogServerPort()); - } + @ClassRule + @ConfiguredWithCode("jcasc.yml") + public static JenkinsConfiguredWithCodeRule r = new JenkinsConfiguredWithCodeRule(); + + @Issue("JENKINS-57232") + @Test + public void should_support_configuration_as_code() { + ExtensionList extensionList = r.jenkins.getExtensionList(AuditTrailPlugin.class); + AuditTrailPlugin plugin = extensionList.get(0); + assertEquals(".*/(?:configSubmit|doUninstall|doDelete|postBuildResult|enable|disable|cancelQueue|stop|toggleLogKeep|doWipeOutWorkspace|createItem|createView|toggleOffline|cancelQuietDown|quietDown|restart|exit|safeExit)", plugin.getPattern()); + assertTrue(plugin.getLogBuildCause()); + assertEquals(3, plugin.getLoggers().size()); + + //first logger + AuditLogger logger = plugin.getLoggers().get(0); + assertTrue(logger instanceof ConsoleAuditLogger); + assertEquals("test", ((ConsoleAuditLogger) logger).getLogPrefix()); + assertEquals(ConsoleAuditLogger.Output.STD_OUT, ((ConsoleAuditLogger) logger).getOutput()); + assertEquals("yyyy-MM-dd HH:mm:ss:SSS", ((ConsoleAuditLogger) logger).getDateFormat()); + + //second logger + logger = plugin.getLoggers().get(1); + assertTrue(logger instanceof LogFileAuditLogger); + assertEquals(10, ((LogFileAuditLogger) logger).getCount()); + assertEquals(666, ((LogFileAuditLogger) logger).getLimit()); + assertEquals("/log/location", ((LogFileAuditLogger) logger).getLog()); + + //third logger + logger = plugin.getLoggers().get(2); + assertEquals("jenkins", ((SyslogAuditLogger) logger).getAppName()); + assertEquals("DAEMON", ((SyslogAuditLogger) logger).getFacility()); + assertEquals("RFC_5424", ((SyslogAuditLogger) logger).getMessageFormat()); + assertEquals("hostname", ((SyslogAuditLogger) logger).getMessageHostname()); + assertEquals("syslog-server", ((SyslogAuditLogger) logger).getSyslogServerHostname()); + assertEquals(514, ((SyslogAuditLogger) logger).getSyslogServerPort()); + } + + @Issue("JENKINS-57232") + @Test + public void should_support_configuration_export() throws Exception { + ConfiguratorRegistry registry = ConfiguratorRegistry.get(); + ConfigurationContext context = new ConfigurationContext(registry); + CNode auditTrailAttribute = getUnclassifiedRoot(context).get("audit-trail"); + + String exported = toYamlString(auditTrailAttribute); + + String expected = toStringFromYamlFile(this, "expected.yml"); + + assertThat(exported, is(expected)); + } } diff --git a/src/test/resources/hudson/plugins/audit_trail/expected.yml b/src/test/resources/hudson/plugins/audit_trail/expected.yml new file mode 100644 index 0000000..4443901 --- /dev/null +++ b/src/test/resources/hudson/plugins/audit_trail/expected.yml @@ -0,0 +1,18 @@ +logBuildCause: true +loggers: +- console: + dateFormat: "yyyy-MM-dd HH:mm:ss:SSS" + logPrefix: "test" + output: STD_OUT +- logFile: + count: 10 + limit: 666 + log: "/log/location" +- syslog: + appName: "jenkins" + facility: "DAEMON" + messageFormat: "RFC_5424" + messageHostname: "hostname" + syslogServerHostname: "syslog-server" + syslogServerPort: 514 +pattern: ".*/(?:configSubmit|doUninstall|doDelete|postBuildResult|enable|disable|cancelQueue|stop|toggleLogKeep|doWipeOutWorkspace|createItem|createView|toggleOffline|cancelQuietDown|quietDown|restart|exit|safeExit)"