-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[JENKINS-52665] Treat plugin dependency mismatches involving snapshots as nonfatal #3551
[JENKINS-52665] Treat plugin dependency mismatches involving snapshots as nonfatal #3551
Conversation
* Similar to {@code org.apache.maven.artifact.ArtifactUtils.isSnapshot}. | ||
*/ | ||
static boolean isSnapshot(@Nonnull String version) { | ||
return version.contains("-SNAPSHOT") || version.matches(".+-[0-9]{8}.[0-9]{6}-[0-9]+"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how close https://updates.jenkins.io/download/plugins/nexus-jenkins-plugin/ comes, I'm not sure this is reality proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what build tool is producing those version numbers but they do not look like regular Maven snapshots to me. I cannot even find the source repo for this…? Anyway we can always expand the list later if there are proven needs; this pattern should cover plugins built using Maven & Incrementals in the normal way, which is a step forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to point out here is that this disables a useful protection mechanism based on a heuristic I'm not sure is reliable -- the format of the version number.
I'm not quite sure what it takes for this to break for real users, but the use case seems somewhat underdefined here (I thought incrementals weren't snapshots).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like just return version.contains("-SNAPSHOT");
covers the case explained in Jira, since if either the actual or minimum version is a Snapshot we suppress the error. Is the purpose of the second pattern to suppress errors if two incremental versions are being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not a match for incremental versions, it is a match for timestamped snapshots like 3.19-20180719.153600-1
(~ 3.19-SNAPSHOT
but a particular instance as deployed to a repository on that date).
Incremental versions would look something like 2.22-rc312.d41f02f66ac5
, and these continue to be strictly compared by the lib-version-number
library, so 311 < 312 < 313. Note that an incremental version that compares as greater than the requested dependency might have come from an unrelated branch and thus not actually be compatible, but it is up to the developer to deal with that (users of e.g. Essentials/Evergreen should never be running anything that has not been merged to master
); what JEP-305 does guarantee (after jenkinsci/incrementals-tools#6 anyway) is that if the actual plugin was built from a commit which is a descendant (in the DAG ancestry sense) of the requested commit (and thus expected to be compatible with the dependency), the actual “revision” number will compare as greater, and so the plugin manager will be satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I totally forgot about timestamped snapshots, thanks for clarifying!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝
public void isSnapshot() { | ||
assertFalse(PluginWrapper.isSnapshot("1.0")); | ||
assertFalse(PluginWrapper.isSnapshot("1.0-alpha-1")); | ||
assertFalse(PluginWrapper.isSnapshot("1.0-rc9999.abc123def456")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test related to Daniel's comment above:
diff --git a/core/src/test/java/hudson/PluginWrapperTest.java b/core/src/test/java/hudson/PluginWrapperTest.java
index e15c803dca..62027f8602 100644
--- a/core/src/test/java/hudson/PluginWrapperTest.java
+++ b/core/src/test/java/hudson/PluginWrapperTest.java
@@ -186,6 +186,7 @@ public class PluginWrapperTest {
assertFalse(PluginWrapper.isSnapshot("1.0"));
assertFalse(PluginWrapper.isSnapshot("1.0-alpha-1"));
assertFalse(PluginWrapper.isSnapshot("1.0-rc9999.abc123def456"));
+ assertFalse(PluginWrapper.isSnapshot("1.2.20170404-163441.794de4c"));
assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT"));
assertTrue(PluginWrapper.isSnapshot("1.0-20180719.153600-1"));
assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT (private-abcd1234-jqhacker)"));
And mvn clean test -Dtest=PluginWrapperTest
still succeeds for me, so LGTM.
Pulling into a top-level thread:
Useful indeed, though hardly critical. After all, we lived without so much as a warning for years.
Maven uses the same heuristic.
You referred to one plugin example with a weird scheme that is not used by any other plugin I have encountered, and ~2k installations. The current code will anyway not treat those versions as snapshots.
Did you read the explanation in JIRA?
They are not. The problem was a comparison between an incremental version and a (locally built— |
@jglick Thanks for the explanation. I agree this makes sense. |
I would like to take a closer look before merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really weak +1. I would vote for having snapshot enforcement as opt-out (System property or whatever). Just because this statement is not 100% correct for the systems I know:
This should never happen in production installations anyway, only during local development.
There are many Jenkins admins who build snapshots and, then, snapshots on the top of snapshots. Incrementals also got some traction in production instances.
@jenkinsci/code-reviewers any other opinions? |
Would it make sense to gate this behavior behind |
I agree with @dwnusbaum , it also makes sense to limit the scope. I put it on hold so that @jglick can review the feedback |
No, because it also affects
And this PR should not break anything for them. It merely suppresses a (relatively recently added) safety switch which may well have already been malfunctioning in that environment.
This fix was developed in order to better support Incrementals! |
OK, let's merge it as is. |
See JENKINS-52665.
Before, an
InjectedTest
failure might look likeThis example is of a plugin depending on a build of jenkinsci/workflow-durable-task-step-plugin#21 & jenkinsci/workflow-job-plugin#27 after switching to a snapshot dependency on local modifications to jenkinsci/workflow-support-plugin#15. With the core patch, the test passes, just printing something like
Alleviates the pressure to finish and integrate jenkinsci/lib-version-number#6.
Proposed changelog entries
@jenkinsci/code-reviewers