Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationalize logging subsystem #400

Merged
merged 7 commits into from
Jan 18, 2023
Merged

Rationalize logging subsystem #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b334867
Rationalize logging subsystem
basil Jan 14, 2023
a2b849a
Update src/main/java/org/jenkins/tools/test/maven/ExternalMavenRunner…
basil Jan 17, 2023
f6dc795
Update src/main/java/org/jenkins/tools/test/maven/ExternalMavenRunner…
basil Jan 17, 2023
5596f1d
Update src/main/java/org/jenkins/tools/test/model/MavenPom.java
basil Jan 17, 2023
1b7dd8d
https://github.com/jenkinsci/plugin-compat-tester/pull/400/files/b334…
basil Jan 17, 2023
a8b1208
Merge remote-tracking branch 'origin/master' into logging
basil Jan 18, 2023
4a3add8
Fix indentation
basil Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions logback.xml
View file
Open in desktop

This file was deleted.

46 changes: 28 additions & 18 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
<properties>
<changelist>999999-SNAPSHOT</changelist>
<gitHubRepo>jenkinsci/plugin-compat-tester</gitHubRepo>
<logbackVersion>1.4.5</logbackVersion>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the removal of Logback this is no longer needed and has therefore been deleted.

<maven.scm.providers.version>1.6</maven.scm.providers.version>
<slf4j.version>2.0.6</slf4j.version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now need to define this version for both slf4j-api and slf4j-jdk14, extracting it to a property to eliminate duplication. This matches what we do in Jenkins core.

<spotbugs.effort>Max</spotbugs.effort>
</properties>

Expand All @@ -46,27 +46,12 @@
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>2.0.6</version>
<version>${slf4j.version}</version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consuming the new property in the original consumer. It will also be consumed later on in a new consumer (slf4j-jdk14).

</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-access</artifactId>
<version>${logbackVersion}</version>
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
<version>${logbackVersion}</version>
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-core</artifactId>
<version>${logbackVersion}</version>
</dependency>
Comment on lines -55 to -69
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the removal of Logback this is no longer needed and has therefore been deleted.

<dependency>
<groupId>com.beust</groupId>
<artifactId>jcommander</artifactId>
Expand All @@ -77,6 +62,11 @@
<artifactId>maven-scm-provider-svnjava</artifactId>
<version>1.12</version>
</dependency>
<dependency>
<groupId>io.jenkins.lib</groupId>
<artifactId>support-log-formatter</artifactId>
<version>1.2</version>
</dependency>
Comment on lines +65 to +69
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using our standard project-wide JUL formatter for consistency with Jenkins core and other Jenkins components.

<dependency>
<groupId>jakarta.servlet</groupId>
<artifactId>jakarta.servlet-api</artifactId>
Expand Down Expand Up @@ -205,6 +195,10 @@
<groupId>commons-jelly</groupId>
<artifactId>commons-jelly-tags-xml</artifactId>
</exclusion>
<exclusion>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
</exclusion>
<exclusion>
<groupId>io.jenkins.stapler</groupId>
<artifactId>jenkins-stapler-support</artifactId>
Expand Down Expand Up @@ -338,6 +332,11 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-jdk14</artifactId>
<version>${slf4j.version}</version>
</dependency>
basil marked this conversation as resolved.
Show resolved Hide resolved
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand Down Expand Up @@ -370,7 +369,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.3.0</version>
<!-- Version specified in parent POM -->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated minor cleanup: I noticed this version was redundant while adding the new entry for maven-surefire-plugin. Note that the version for maven-shade-plugin does need to be specified, since it is not included in the parent POM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkinsci/pom#371

<configuration>
<archive>
<manifestEntries>
Expand Down Expand Up @@ -417,6 +416,17 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<!-- Version specified in parent POM -->
<configuration>
<systemPropertyVariables>
<java.util.logging.config.class>org.jenkins.tools.test.logging.LoggingConfiguration</java.util.logging.config.class>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to get the tests to use our custom logging configuration, which is useful when debugging tests in an IDE. For production this is enabled by a static initializer in PluginCompatTesterCli.

</systemPropertyVariables>
<runOrder>alphabetical</runOrder>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated code cleanup: getting the tests to be run in a consistent order. The plugin parent POM has this, but the jenkinsci/pom does not (though Jenkins core specifies it). It would be nice to move this up from here and from core into jenkinsci/pom, but I am explicitly not taking on this task as part of the present PR.

</configuration>
</plugin>
</plugins>
</build>
</project>
88 changes: 41 additions & 47 deletions src/main/java/org/jenkins/tools/test/PluginCompatTester.java
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.List;
import java.util.Map;
import org.codehaus.plexus.PlexusContainerException;
import org.jenkins.tools.test.logging.LoggingConfiguration;
import org.jenkins.tools.test.model.PluginCompatTesterConfig;
import org.jenkins.tools.test.exception.PomExecutionException;
import org.jenkins.tools.test.model.TestStatus;
Expand All @@ -47,6 +48,14 @@
*/
public class PluginCompatTesterCli {

static {
String configFile = System.getProperty("java.util.logging.config.file");
String configClass = System.getProperty("java.util.logging.config.class");
if (configClass == null && configFile == null) {
new LoggingConfiguration();
}
}
Comment on lines +51 to +57
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to get production to use our custom logging configuration (I tried to find a way to add a MANIFEST.MF entry to do this, but there was none). For tests this is enabled by adding -Djava.util.logging.config.class=org.jenkins.tools.test.logging.LoggingConfiguration in the Surefire configuration.


@SuppressFBWarnings(
value = "NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE",
justification =
Expand Down
22 changes: 13 additions & 9 deletions src/main/java/org/jenkins/tools/test/hook/AbstractMultiParentHook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.apache.maven.scm.ScmException;
import org.apache.maven.scm.ScmFileSet;
Expand All @@ -22,10 +24,12 @@
import org.jenkins.tools.test.model.hook.PluginCompatTesterHookBeforeCheckout;

/**
* Utility class to ease create simple hooks for multimodule projects
* Utility class to ease create simple hooks for multi-module projects
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Maven documentation (not that it is a particularly consistent source of correct spelling) spells this with a hyphen, so do the same here.

*/
public abstract class AbstractMultiParentHook extends PluginCompatTesterHookBeforeCheckout {

private static final Logger LOGGER = Logger.getLogger(AbstractMultiParentHook.class.getName());

protected boolean firstRun = true;

private PomData pomData;
Expand All @@ -40,10 +44,10 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception
boolean shouldExecuteHook = config.getLocalCheckoutDir() == null || !config.getLocalCheckoutDir().exists();

if (shouldExecuteHook) {
System.out.println("Executing Hook for " + getParentProjectName());
LOGGER.log(Level.INFO, "Executing hook for {0}", getParentProjectName());
// Determine if we need to run the download; only run for first identified plugin in the series
if (firstRun) {
System.out.println("Preparing for Multimodule checkout");
LOGGER.log(Level.INFO, "Preparing for multi-module checkout");

// Checkout to the parent directory. All other processes will be on the child directory
File parentPath = new File(config.workDirectory.getAbsolutePath() + "/" + getParentFolder());
Expand All @@ -52,10 +56,10 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception
String scmTag;
if (pomData.getScmTag() != null) {
scmTag = pomData.getScmTag();
System.out.println(String.format("Using SCM tag '%s' from POM.", scmTag));
LOGGER.log(Level.INFO, "Using SCM tag {0} from POM", scmTag);
} else {
scmTag = getParentProjectName() + "-" + currentPlugin.version;
System.out.println(String.format("POM did not provide an SCM tag. Inferring tag '%s'.", scmTag));
LOGGER.log(Level.INFO, "POM did not provide an SCM tag; inferring tag {0}", scmTag);
}
// Like PluginCompatTester.cloneFromSCM but with subdirectories trimmed:
cloneFromSCM(currentPlugin, parentPath, scmTag, getUrl(), config.getFallbackGitHubOrganization());
Expand All @@ -68,7 +72,7 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception
// Change the "download"" directory; after download, it's simply used for reference
File childPath = new File(config.workDirectory.getAbsolutePath() + "/" + getParentFolder() + "/" + getPluginFolderName(currentPlugin));

System.out.println("Child path for " + currentPlugin.getDisplayName() + " " + childPath);
LOGGER.log(Level.INFO, "Child path for {0}: {1}", new Object[]{currentPlugin.getDisplayName(), childPath.getPath()});
moreInfo.put("checkoutDir", childPath);
moreInfo.put("pluginDir", childPath);
moreInfo.put("parentFolder", getParentFolder());
Expand Down Expand Up @@ -96,7 +100,7 @@ private void cloneFromSCM(UpdateSite.Plugin currentPlugin, File parentPath, Stri
if (connectionURL != null) {
connectionURL = connectionURL.replace("git://", "https://"); // See: https://github.blog/2021-09-01-improving-git-protocol-security-github/
}
System.out.println("Checking out from SCM connection URL: " + connectionURL + " (" + getParentProjectName() + "-" + currentPlugin.version + ") at tag " + scmTag);
LOGGER.log(Level.INFO, "Checking out from SCM connection URL {0}: {1} ({2}-{3}) at tag {4}", new Object[]{connectionURL, getParentProjectName(), currentPlugin.version, scmTag});
if (parentPath.isDirectory()) {
FileUtils.deleteDirectory(parentPath);
}
Expand All @@ -122,11 +126,11 @@ public String getUrl() {

protected void configureLocalCheckOut(UpdateSite.Plugin currentPlugin, File localCheckoutDir, Map<String, Object> moreInfo) {
// Do nothing to keep compatibility with pre-existing Hooks
System.out.println("Ignoring localCheckoutDir for " + currentPlugin.getDisplayName());
LOGGER.log(Level.INFO, "Ignoring local checkout directory for {0}", currentPlugin.getDisplayName());
}

/**
* Returns the folder where the multimodule project parent will be checked out
* Returns the folder where the multi-module project parent will be checked out
*/
protected abstract String getParentFolder();

Expand Down
10 changes: 7 additions & 3 deletions src/main/java/org/jenkins/tools/test/hook/ExampleMultiParent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.apache.maven.scm.ScmFileSet;
import org.apache.maven.scm.ScmTag;
import org.apache.maven.scm.command.checkout.CheckOutScmResult;
Expand All @@ -18,9 +20,11 @@
* each plugin is in its own repository, these plugins automatically fail since they are "not
* found".
*
* <p>This is an example of what needs to change to handle multimodule parents.
* <p>This is an example of what needs to change to handle multi-module parents.
*/
public class ExampleMultiParent { //extends PluginCompatTesterHookBeforeCheckout {
private static final Logger LOGGER = Logger.getLogger(ExampleMultiParent.class.getName());

private String parentUrl = "scm:git:[email protected]:jenkinsci/parent_repo.git";
private String parentName = "parent_repo";
private List<String> allBundlePlugins = List.of("possible", "plugins");
Expand Down Expand Up @@ -49,12 +53,12 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception

// Determine if we need to run the download; only run for first identified plugin in the series
if(firstRun){
System.out.println("Preparing for Multimodule checkout.");
LOGGER.log(Level.INFO, "Preparing for multi-module checkout");

// Checkout to the parent directory. All other processes will be on the child directory
File parentPath = new File(config.workDirectory.getAbsolutePath()+"/"+parentName);

System.out.println("Checking out from SCM connection URL : "+parentUrl+" ("+parentName+"-"+currentPlugin.version+")");
LOGGER.log(Level.INFO, "Checking out from SCM connection URL: {0} ({1}-{2})", new Object[]{parentUrl, parentName, currentPlugin.version});
ScmManager scmManager = SCMManagerFactory.getInstance().createScmManager();
ScmRepository repository = scmManager.makeScmRepository(parentUrl);
CheckOutScmResult result = scmManager.checkOut(repository, new ScmFileSet(parentPath), new ScmTag(parentName+"-"+currentPlugin.version));
Expand Down
23 changes: 13 additions & 10 deletions src/main/java/org/jenkins/tools/test/hook/MultiParentCompileHook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import java.nio.file.StandardCopyOption;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Stream;

import org.apache.commons.io.FileUtils;
Expand All @@ -26,28 +28,30 @@

public class MultiParentCompileHook extends PluginCompatTesterHookBeforeCompile {

private static final Logger LOGGER = Logger.getLogger(MultiParentCompileHook.class.getName());

protected MavenRunner runner;
protected MavenRunner.Config mavenConfig;

public static final String ESLINTRC = ".eslintrc";

public MultiParentCompileHook() {
System.out.println("Loaded multi-parent compile hook");
LOGGER.log(Level.INFO, "Loaded multi-parent compile hook");
}


@Override
@SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "silly rule")
public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception {
try {
System.out.println("Executing multi-parent compile hook");
LOGGER.log(Level.INFO, "Executing multi-parent compile hook");
PluginCompatTesterConfig config = (PluginCompatTesterConfig) moreInfo.get("config");

runner = new ExternalMavenRunner(config.getExternalMaven());
mavenConfig = getMavenConfig(config);

File pluginDir = (File) moreInfo.get("pluginDir");
System.out.println("Plugin dir is " + pluginDir);
LOGGER.log(Level.INFO, "Plugin dir is {0}", pluginDir);

File localCheckoutDir = config.getLocalCheckoutDir();
if (localCheckoutDir != null) {
Expand All @@ -73,12 +77,11 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception
moreInfo.put(OVERRIDE_DEFAULT_COMPILE, true);
}

System.out.println("Executed multi-parent compile hook");
LOGGER.log(Level.INFO, "Executed multi-parent compile hook");
return moreInfo;
// Exceptions get swallowed, so we print to console here and rethrow again
} catch (Exception e) {
System.out.println("Exception executing hook");
System.out.println(e);
LOGGER.log(Level.WARNING, "Exception executing hook", e);
throw e;
}
}
Expand Down Expand Up @@ -143,12 +146,12 @@ private boolean isSnapshotMultiParentPlugin(String parentFolder, File path, File
return false;
}
if (!path.getAbsolutePath().contains(parentFolder)) {
System.out.println(String.format("Something is really wrong here! parentFolder:%s not present in path %s", parentFolder, path.getAbsolutePath()));
LOGGER.log(Level.WARNING, "Parent folder {0} not present in path {1}", new Object[]{parentFolder, path.getAbsolutePath()});
return false;
}
File parentFile = path.getParentFile();
if (!StringUtils.equals(parentFolder, parentFile.getName())) {
System.out.println(String.format("Something is really wrong here! %s is not the parent folder of %s", parentFolder, path.getAbsolutePath()));
LOGGER.log(Level.WARNING, "{0} is not the parent folder of {1}", new Object[]{parentFolder, path.getAbsolutePath()});
return false;
}

Expand All @@ -160,9 +163,9 @@ private boolean isSnapshotMultiParentPlugin(String parentFolder, File path, File


private File setupCompileResources(File path) throws IOException {
System.out.println("Cleaning up node modules if necessary");
LOGGER.log(Level.INFO, "Cleaning up node modules if necessary");
removeNodeFolders(path);
System.out.println("Compile plugin log in " + path);
LOGGER.log(Level.INFO, "Plugin compilation log directory: {0}", path);
return new File(path + "/compilePluginLog.log");
}

Expand Down
13 changes: 8 additions & 5 deletions src/main/java/org/jenkins/tools/test/hook/NodeCleanupBeforeCompileHook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@
import java.io.File;
import java.io.IOException;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;

public class NodeCleanupBeforeCompileHook extends PluginCompatTesterHookBeforeCompile {

private static final Logger LOGGER = Logger.getLogger(NodeCleanupBeforeCompileHook.class.getName());

@Override
public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception {
PluginCompatTesterConfig config = (PluginCompatTesterConfig) moreInfo.get("config");
Expand All @@ -18,16 +22,15 @@ public Map<String, Object> action(Map<String, Object> moreInfo) throws Exception
if (shouldExecuteHook) {
File pluginDir = (File) moreInfo.get("pluginDir");
try {
System.out.println("Executing node and node_modules cleanup hook");
LOGGER.log(Level.INFO, "Executing node and node_modules cleanup hook");
compile(pluginDir);
return moreInfo;
} catch (Exception e) {
System.out.println("Exception executing hook");
System.out.println(e);
LOGGER.log(Level.WARNING, "Exception executing hook", e);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this logging and throwing is itself an anti-pattern that could be cleaned up in a future PR that deals with error handling and propagation, but is explicitly out of scope for this PR.

throw e;
}
} else {
System.out.println("Hook not triggered. Continuing.");
LOGGER.log(Level.INFO, "Hook not triggered; continuing");
return moreInfo;
}
}
Expand All @@ -37,7 +40,7 @@ public void validate(Map<String, Object> toCheck) {
}

private void compile(File path) throws IOException {
System.out.println("Calling removeNodeFolders");
LOGGER.log(Level.INFO, "Calling removeNodeFolders");
removeNodeFolders(path);
}

Expand Down
Loading