[JENKINS-49635] Support new VirtualFile.toExternalURL API #60
Conversation
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Blocking this because of the unacceptable core bump -- see comments in upstream PR.
oleg-nenashev
left a comment
oleg-nenashev
left a comment
There was a problem hiding this comment.
🐝 I am afraid about the Binary compatibility breakage, but it's up to plugin maintainers
| * @author Kohsuke Kawaguchi | ||
| */ | ||
| public class ArtifactUnarchiverStepExecution extends SynchronousNonBlockingStepExecution<List<FilePath>> { | ||
| public class ArtifactUnarchiverStepExecution extends SynchronousNonBlockingStepExecution<Void> { |
There was a problem hiding this comment.
Not binary compatible, needs to be Restricted at least
| @@ -1 +1 @@ | |||
| buildPlugin(jenkinsVersions: [null, '2.32.3']) | |||
| buildPlugin() | |||
There was a problem hiding this comment.
No, the baseline is newer than that.
jglick
changed the title
|
Will keep this |
abayer
left a comment
abayer
left a comment
There was a problem hiding this comment.
Looks fine, release will have to wait for an LTS, though.
| public class ArtifactArchiverStepTest extends Assert { | ||
| public class ArtifactArchiverStepTest { | ||
|
|
||
| @ClassRule public static BuildWatcher watcher = new BuildWatcher(); |
There was a problem hiding this comment.
Just so that the test log includes the log from the build run at line 90.
| j.createSlave("remote1", null, null); | ||
| j.createSlave("remote2", null, null); | ||
| WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition("node('remote1') {writeFile file: 'x', text: 'contents'; archiveArtifacts 'x'}; node('remote2') {unarchive mapping: [x: 'x']; echo(/loaded ${readFile('x')}/)}", true)); |
There was a problem hiding this comment.
No, because that behavior is not being defined in this PR—it would be overridden in the plugin providing the ArtifactManagerFactory.
| private FilePath copy(VirtualFile src, FilePath dst, TaskListener listener) throws IOException, InterruptedException { | ||
| URL u = src.toExternalURL(); | ||
| if (u != null) { | ||
| new RobustHTTPClient().copyFromRemotely(dst, u, listener); |
There was a problem hiding this comment.
Caveat that I need to look into what guarantees this is offering and how thread interruption is handled in the RobustHTTPClient
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
AFAICT looks reasonable, with the exception of needing to look at the RobustHTTPClient impl
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Looks fine now, but we need to get the base branches set up for the separate stable & bleeding-edge branches.
@jglick Please could you give me a hand with the permissions there?
Starting a run through the standing PRs to make sure we have integrated what we can before cutting the split branches.
5 participants
Downstream of jenkinsci/workflow-api-plugin#67 & jenkinsci/jenkins#3302 & jenkinsci/workflow-step-api-plugin#33.
@reviewbybees