Fleshing out backwards compatibility section of JEP-202 #98
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,16 +154,76 @@ The S3 implementation also uses http://jclouds.apache.org[Apache JClouds] that a | |
| == Backwards Compatibility | ||
|
|
||
| Existing plugins using `ArtifactManager` API will continue to work using the new selected implementation. | ||
| However, there are two classes of potential incompatibility. | ||
|
|
||
| === File-oriented artifact reference | ||
|
|
||
| Various plugins call deprecated APIs which assume that build artifacts are stored as files inside the master’s build directory. | ||
| These would already have been broken for users of the _Compress Artifacts_ plugin, but that is rarely used, whereas we are proposing lots of people run with the S3 artifact manager. | ||
| We could add telemetry so that such calls produce a warning in the system log, at least when the build actually does have a custom artifact manager selected. | ||
|
|
||
| As seen in | ||
| link:https://ci.jenkins.io/job/Infra/job/deprecated-usage-in-plugins/job/master/lastSuccessfulBuild/artifact/output/usage-by-api.html#hudson_model_Run_getArtifactsDir__Ljava_io_File_[this report], | ||
| there are a number of plugins on the usual update center still calling `Run.getArtifactsDir()` and/or `Run.Artifact.getFile()`, | ||
| despite the fact that these methods were deprecated in Jenkins 1.531 in 2013 as part of | ||
| link:https://issues.jenkins-ci.org/browse/JENKINS-17236[JENKINS-17236]. | ||
| These include: | ||
|
|
||
| [cols="5,>",options="header",width="50%"] | ||
| |============================ | ||
|
Member
There was a problem hiding this comment. Would it make sense to create RFE tickets and reference them from JEP? maybe it's rather preferable to it in a Wiki page so that we could do it dynamically (like https://wiki.jenkins.io/display/JENKINS/Plugins+affected+by+fix+for+JEP-200 or https://wiki.jenkins.io/display/JENKINS/Plugins+affected+by+fix+for+SECURITY-170) Then the warning message for old API could redirect people to that page
Member
Author
There was a problem hiding this comment. I did not consider it worth bothering given the limited impact and the low installation counts. Would be possible if adding core logging, of course. At any rate, that does not seem to have much bearing on the text of the JEP. |
||
| |Plugin|Installations | ||
| |link:https://plugins.jenkins.io/allure-jenkins-plugin[Allure]|2593 | ||
| |link:https://plugins.jenkins.io/artifact-diff-plugin[Artifact diff]|433 | ||
| |link:https://plugins.jenkins.io/copyartifact[Copy Artifact]|36641 | ||
| |link:https://plugins.jenkins.io/cucumber-perf[cucumber-perf]|919 | ||
| |link:https://plugins.jenkins.io/deployer-framework[Deployer Framework]|703 | ||
| |link:https://plugins.jenkins.io/weblogic-deployer-plugin[Deploy WebLogic]|1250 | ||
| |link:https://plugins.jenkins.io/http-post[HTTP POST]|1498 | ||
| |link:https://plugins.jenkins.io/repository[Maven Repository Server]|2023 | ||
| |link:https://plugins.jenkins.io/mdt-deployment[MDT Deployment]|80 | ||
| |link:https://plugins.jenkins.io/neoload-jenkins-plugin[NeoLoad]|163 | ||
| |link:https://plugins.jenkins.io/perfectomobile[Perfecto Mobile]|174 | ||
| |link:https://plugins.jenkins.io/protecode-sc[Protecode SC]|26 | ||
| |link:https://plugins.jenkins.io/summary_report[Summary Display]|1714 | ||
| |link:https://plugins.jenkins.io/webload[WebLOAD Load Testing]|34 | ||
| |============================ | ||
|
|
||
| By far the most popular of these, _Copy Artifact_, | ||
| is scheduled to be made compatible with this JEP as part of the reference implementation. | ||
| (The first stage of that fix implements a longstanding RFE | ||
| link:https://issues.jenkins-ci.org/browse/JENKINS-22637[JENKINS-22637], | ||
| originally filed for interoperability with _Compress Artifacts_. | ||
| The second stage of the fix makes use of core APIs introduced in this JEP.) | ||
|
|
||
| The effect of calling the deprecated APIs when a cloud-based artifact manager is in use | ||
| will vary by the plugin’s particular logic. | ||
| In some cases, it may simply appear as if the build had no artifacts. | ||
| JENKINS-22637 describes an error message when attempting to use _Copy Artifact_. | ||
| As another example, _Artifact diff_ will display a sidebar link as usual, | ||
| but when clicked the rendered diff is empty, and the system log reports: | ||
|
|
||
| ---- | ||
| … org.jenkinsci.plugins.artifactdiff.FilePathDiff$Entry getStream | ||
| INFO: java.nio.file.NoSuchFileException: /var/jenkins_home/jobs/someproject/builds/123/archive/somefile.txt | ||
| ---- | ||
|
|
||
| === Master-based file streaming | ||
|
|
||
| Some plugins using ``VirtualFile``s corresponding to build artifacts are still calling `open` | ||
|
Member
There was a problem hiding this comment. wow, didn't know about this syntax (double-single-quote) |
||
| and then passing the stream to an agent or copying it to an HTTP response. | ||
| This will work, but will be very expensive when using S3 storage. They need to be updated to call `VirtualFile.toExternalURL`. | ||
| Finding a list of such plugins is more difficult since `open` is not deprecated. | ||
| (Its use is appropriate as a fallback when `toExternalURL` is unavailable, | ||
| or when the desired behavior is for artifact contents to be read by the Jenkins master process anyway.) | ||
| Code inspection from | ||
| link:https://github.com/search?q=user%3Ajenkinsci+VirtualFile&type=Code[this search] | ||
| turns up the following possible issues: | ||
|
|
||
| [cols="5,>",options="header",width="50%"] | ||
| |============================ | ||
| |Plugin|Installations | ||
| |link:https://plugins.jenkins.io/maven-plugin[Maven Integration]|124783 | ||
|
Member
There was a problem hiding this comment. Oh, should we rename it to die-hard-plugin? :D |
||
| |============================ | ||
|
|
||
| == Security | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Maybe it makes sense to modify the API methods and to print WARNINGs if these APIs are used with custom ArtifactManager configurations. Then it can be somehow routed to telemetry in JEP-304
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.
Yes, was already suggested in this JEP, but does not do much good until JEP-304 is live I suppose.