Clean up hooks #403

basil · 2023-01-14T00:11:20Z

Problem

In a number of places, this code base assumed that the parent POM was available, an assumption that is no longer valid after the introduction of incrementals and flatten-maven-plugin. In a few places, the existing code was printing ugly warnings when the parent POM was unavailable and proceeding down an alternate code path.

Solution

This PR begins to address the problem by dealing with AbstractMultiParentHook and its descendants. (#400 turns down the log level for some other cases, and when both of these PRs are merged I will look at what remains and clean up the rest.) There is no need to rely on the parent POM in any of these cases, since an alternative paradigm completely suffices for open-source Jenkins plugin: check that the group ID and artifact ID are as expected for the plugin (see the "gav" section of the Update Center JSON) and that the packaging type is "hpi".

While we were here, we also clarified the getParentFolder and getParentProjectName and made existing implementations consistent. These could probably be removed entirely in the future but we are not going that far in this PR. Once this is merged I will investigate that and file a separate issue if it makes sense.

Testing done

Tested this in context in jenkinsci/bom.

basil · 2023-01-14T21:02:01Z

src/main/java/org/jenkins/tools/test/hook/AbstractMultiParentHook.java

+     * Return the folder where the multi-module project will be checked out. This should be the name
+     * of the plugin's Git repository.


This doesn't really matter one way or another, but to be consistent with plugins that are not multi-module, recommend the name of the Git repository.

do we ever have a valid scenario where we have a multi module project with > 1 plugin where we would have different versions of those plugins to test in the release?
if that is the case does using the git repo name cause issues in testing (as you can only have one version checked out at once and then depending what you are running you may have a plugin from both the war and the reactor.

I am not aware of such a scenario and it doesn't make sense to me that one would ever be mixing plugin versions from the same multi-module project.

basil · 2023-01-14T21:02:22Z

src/main/java/org/jenkins/tools/test/hook/AbstractMultiParentHook.java

+     * Return the prefix to the SCM tag (usually the artifact ID of the base module). This will be
+     * used to form the checkout tag with the format {@code parentProjectName-version} in the (highly
+     * unlikely, and impossible for incrementalified plugins) event that the SCM tag is missing from
+     * the plugin's POM.


This is mostly useless because it is only used in a fallback code path, but at least document the convention that would make that fallback path work.

basil · 2023-01-14T21:02:31Z

src/main/java/org/jenkins/tools/test/hook/AwsJavaSdkHook.java


    @Override
    protected String getParentFolder() {
-        return "aws-java-sdk";
+        return "aws-java-sdk-plugin";


Consistent with the Javadoc.

basil · 2023-01-14T21:03:14Z

src/main/java/org/jenkins/tools/test/hook/AwsJavaSdkHook.java

+        PomData data = (PomData) info.get("pomData");
+        return ("org.jenkins-ci.plugins".equals(data.groupId)
+                        && "aws-java-sdk".equals(data.artifactId)
+                        && "hpi".equals(data.getPackaging()))
+                || ("org.jenkins-ci.plugins.aws-java-sdk".equals(data.groupId)
+                        && data.artifactId.startsWith("aws-java-sdk")
+                        && "hpi".equals(data.getPackaging()));


From update-center-sql:

sqlite> select gav from plugins where name like 'aws-java-sdk%'; org.jenkins-ci.plugins:aws-java-sdk:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-cloudformation:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-codebuild:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-ec2:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-ecr:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-ecs:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-efs:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-elasticbeanstalk:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-iam:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-logs:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-minimal:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-sns:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-sqs:1.12.287-357.vf82d85a_6eefd org.jenkins-ci.plugins.aws-java-sdk:aws-java-sdk-ssm:1.12.287-357.vf82d85a_6eefd

basil · 2023-01-14T21:03:27Z

src/main/java/org/jenkins/tools/test/hook/BlueOceanHook.java

@@ -11,11 +9,9 @@
 */
 public class BlueOceanHook extends AbstractMultiParentHook {

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


Removing unused code.

basil · 2023-01-14T21:07:59Z

src/main/java/org/jenkins/tools/test/hook/StructsHook.java

-import java.util.logging.Logger;
-import org.jenkins.tools.test.model.PomData;
-
-public class StructsHook extends AbstractMultiParentHook {


Structs was flattened a while ago, and this hook was only needed to test older versions where it is not flattened. Since I do not think there are any consumers left that would be trying to run PCT on old versions of structs, deleting this hook.

need to update this javadoc comment?

Sure, why not

Done in commit 57170b6

basil · 2023-01-14T21:08:07Z

src/main/java/org/jenkins/tools/test/hook/SwarmHook.java

    @Override
    protected String getParentFolder() {
-        return "swarm";
+        return "swarm-plugin";


Consistent with the Javadoc.

basil · 2023-01-14T21:08:24Z

src/main/java/org/jenkins/tools/test/hook/SwarmHook.java

+        PomData data = (PomData) info.get("pomData");
+        return "org.jenkins-ci.plugins".equals(data.groupId)
+                && "swarm".equals(data.artifactId)
+                && "hpi".equals(data.getPackaging());


From update-center-sql:

sqlite> select gav from plugins where name like 'swarm%'; org.jenkins-ci.plugins:swarm:3.39

basil · 2023-01-14T21:09:01Z

src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHook.java

@@ -13,7 +14,7 @@ protected String getParentFolder() {

    @Override
    protected String getParentProjectName() {
-        return "workflow-cps-parent";
+        return "workflow-cps";


Consistent with the tagging scheme of this repository, not that it matters since this was a fallback code path. Since this file was added recently, the mistake here just serves to illustrate how poor the old Javadoc was (corrected in this PR).

basil · 2023-01-14T21:09:36Z

src/main/java/org/jenkins/tools/test/hook/WorkflowCpsHook.java

+            return "org.jenkins-ci.plugins.workflow".equals(data.groupId)
+                    && "workflow-cps".equals(data.artifactId)
+                    && pluginVersion.isNewerThan(multiModuleSince)
+                    && "hpi".equals(data.getPackaging());


Following the same convention used elsewhere by checking the group ID, artifact ID, and packaging type. In this case we also need to check the version to see if it is one where the plugin is single-module or multi-module.

…_r1072397461

jtnord

code looks ok - will setup a job to test this with our proprietary code.

basil · 2023-01-17T19:40:04Z

code looks ok - will setup a job to test this with our proprietary code.

Thanks, let me know when the build is green and I will squash merge this

jtnord · 2023-01-18T13:39:54Z

downstream closed source build is passing - this is good to merge.

Clean up hooks

7e9e0f7

basil added the work-in-progress label Jan 14, 2023

basil added a commit to basil/bom that referenced this pull request Jan 14, 2023

Test jenkinsci/plugin-compat-tester#403

22f74c4

basil added a commit to basil/bom that referenced this pull request Jan 14, 2023

Test jenkinsci/plugin-compat-tester#403

73e808e

basil added internal and removed work-in-progress labels Jan 14, 2023

basil marked this pull request as ready for review January 14, 2023 21:01

basil requested a review from a team as a code owner January 14, 2023 21:01

basil requested a review from jtnord January 14, 2023 21:01

basil commented Jan 14, 2023

View reviewed changes

https://github.com/jenkinsci/plugin-compat-tester/pull/403#discussion…

57170b6

…_r1072397461

jtnord approved these changes Jan 17, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into hooks-cleanup

c2a09c9

basil merged commit fc56d5a into master Jan 18, 2023

basil deleted the hooks-cleanup branch January 18, 2023 15:55

basil mentioned this pull request Jan 18, 2023

Upgrade PCT to latest jenkinsci/bom#1683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up hooks #403

Clean up hooks #403

basil commented Jan 14, 2023 •

edited

basil Jan 14, 2023

jtnord Jan 17, 2023

basil Jan 17, 2023

basil Jan 14, 2023

basil Jan 14, 2023

basil Jan 14, 2023

basil Jan 14, 2023

basil Jan 14, 2023

jtnord Jan 17, 2023

basil Jan 17, 2023

basil Jan 17, 2023

basil Jan 14, 2023

basil Jan 14, 2023

basil Jan 14, 2023

basil Jan 14, 2023

jtnord left a comment

basil commented Jan 17, 2023

jtnord commented Jan 18, 2023

		* Return the folder where the multi-module project will be checked out. This should be the name
		* of the plugin's Git repository.

Clean up hooks #403

Clean up hooks #403

Conversation

basil commented Jan 14, 2023 • edited

Problem

Solution

Testing done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

basil commented Jan 17, 2023

jtnord commented Jan 18, 2023

basil commented Jan 14, 2023 •

edited