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.

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

Remove support for plugins without an SCM tag #453

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Remove support for plugins without an SCM tag #453

merged 1 commit into from
Feb 23, 2023

Conversation

basil
Copy link
Member

@basil basil commented Feb 9, 2023

Fixes #452 and removes the temporary workaround added in #451. Plugins in jenkinsci/bom should be OK but there are a handful (less than 5) proprietary plugins that are missing SCM sections. It shouldn't be too hard to patch them up with SCM sections and do releases, at which point this code can be simplified.

@basil basil added the removed label Feb 9, 2023
@basil basil requested a review from jtnord February 9, 2023 00:12
@basil basil requested a review from a team as a code owner February 9, 2023 00:12
@basil basil force-pushed the fallback branch 2 times, most recently from 1932ac1 to 56743a6 Compare February 13, 2023 17:04
Comment on lines -82 to -87
public String getUrl() {
return pomData.getConnectionUrl().replaceFirst("^(.+github[.]com/[^/]+/[^/]+)/.+", "$1");
}
Copy link
Member

Choose a reason for hiding this comment

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

@basil basil force-pushed the fallback branch 2 times, most recently from 52f3e50 to 94e62f9 Compare February 15, 2023 15:27
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

I don't think this is correct as-is

It is perfectly valid for multi-module maven projects to not define the scm for every single module in the reactor (it is only needed when the module needs to have a different scm entry due to using a parent that is not inside the reactor for example).

maven-release-plugin will ensure any scm tag is correct if provided, however it does not require it and if it comes from the parent (along with the not appending the child path) then it will not create it.

Forcing multi-module projects to have to put scm in their modules where it is the same as in the parent would be an anti pattern for maven.

This is presumably working in OSS for the plugins that have tested because they are all incrementalified and hence a flattened pom which maven-release-plugin will not do.

I will dig into this a bit more with multi-module projects once I have a release removing the garbage SCM that https://github.com/jenkinsci/plugin-compat-tester/pull/453/files#diff-e8fe89546f150d9d1512f4cc0fa113c391769c967cbe2cf8add2764bb522798bL87 fixes.

@basil
Copy link
Member Author

basil commented Feb 15, 2023

Yeah, I understand that having an SCM section for each module might not be strictly required in the Maven world, but it simplifies the code in PCT so I think it would be a reasonable "extra" restriction to place on Jenkins plugins, especially since the vast majority of existing Jenkins plugins already comply. In other words, I'd much rather have the clean code in this PR that is easy to reason about than to keep the current messy code just to accommodate a few plugins.

@jtnord
Copy link
Member

jtnord commented Feb 15, 2023

I'm not fully with you here - we can keep the majority of the clean-up here the only required change is to retain 2 null checks

diff --git a/src/main/java/org/jenkins/tools/test/model/PluginRemoting.java b/src/main/java/org/jenkins/tools/test/model/PluginRemoting.java
index 1e97437..aa3e209 100644
--- a/src/main/java/org/jenkins/tools/test/model/PluginRemoting.java
+++ b/src/main/java/org/jenkins/tools/test/model/PluginRemoting.java
@@ -127,8 +127,8 @@ public class PluginRemoting {
                 model.getArtifactId(),
                 model.getPackaging(),
                 // scm may contain properties so it needs to be resolved.
-                interpolateString(model.getScm().getConnection(), model.getArtifactId()),
-                model.getScm().getTag(),
+                model.getScm() == null ? "MISSING_SCM_INFOMATION" : interpolateString(model.getScm().getConnection(), model.getArtifactId()),
+                model.getScm() == null ? "MISSING_SCM_INFOMATION"  : model.getScm().getTag(),
                 parent,
                 model.getGroupId());
     }

with that a hook can update pomData in the moreInfo map before checkout. We still require a hook for multi-module projects in order to know where the plugin code is within the repo (and to flag it as coming from a multi-module project)

e.g.

+    @Override
+    public Map<String, Object> action(Map<String, Object> moreInfo) throws PluginSourcesUnavailableException {
+        PomData pomData = (PomData) moreInfo.get("pomData");
+        UpdateSite.Plugin plugin = (Plugin) moreInfo.get("plugin");
+        PomData pd = new PomData(pomData.artifactId, pomData.getPackaging(),
+                "scm:git:ssh://git@github.com/myorg/my-multiplugin.git",
+                "mymulti-"+plugin.version,
+                pomData.parent, pomData.groupId);
+        moreInfo.put("pomData", pd);
+        return super.action(moreInfo);
+    }

with the above change and without any hook to set the tag/url then rather than a NullPointerException we also get a nicer PluginSourcesUnavailableException like the following.

2023-02-15 19:33:23.906+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository MISSING_SCM_INFOMATION at MISSING_SCM_INFOMATION
2023-02-15 19:33:24.828+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository MISSING_SCM_INFOMATION at MISSING_SCM_INFOMATION
2023-02-15 19:33:25.245+0000 [id=1]     INFO    o.j.t.test.PluginCompatTester#cloneImpl: Checking out from git repository MISSING_SCM_INFOMATION at MISSING_SCM_INFOMATION
org.jenkins.tools.test.exception.PluginSourcesUnavailableException: git fetch origin failed with exit status 128: fatal: 'MISSING_SCM_INFOMATION' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
        at org.jenkins.tools.test.PluginCompatTester.cloneImpl(PluginCompatTester.java:563)
        at org.jenkins.tools.test.PluginCompatTester.cloneFromScm(PluginCompatTester.java:465)

@basil
Copy link
Member Author

basil commented Feb 15, 2023

This is going against my design which is to eventually get rid of all multi-module hooks. Since I am the one implementing this, I think I should get to make the decision. I do not think it is very much to ask for every plugin to publish a POM with an <scm> section and there are only a handful of plugins that do not already comply.

@jtnord
Copy link
Member

jtnord commented Feb 16, 2023

Since I am the one implementing this, I think I should get to make the decision

noted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove support for missing <scm> section
3 participants