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

[JENKINS-22637] ArtifactManager support #99

Merged
merged 5 commits into from Apr 29, 2018

Conversation

Projects
None yet
3 participants
@jglick
Copy link
Member

commented Feb 20, 2018

JENKINS-22637: supporting custom ArtifactManagers via VirtualFile. Unlike #71 the goal here is to make just this change with as little impact on existing installations as possible.

  • basic refactoring
  • integration test with the compress-artifacts plugin (14 votes)
  • restore support for explicit excludes (1 vote)
  • restore support for symlinks (2 votes): #100
  • restore support for file attribute handling (11 votes): #100
  • restore support for default excludes (1 vote): #100

To handle symlinks and file attributes may require new core APIs—TBD. Really if you care about platform-specific stuff like this you should create a tarball and archive that instead, since you cannot guarantee that the Jenkins master will preserve these nuances. Similarly, suppressing of default excludes is desirable but might just be treated as a core bug fix, and again it is going to be safer and much more efficient to create a ZIP or tarball of all your little files.

I also plan to

but this will certainly require a new core baseline and might better be filed as an add-on PR.

Note that I am dropping the Copier extension point here. In practice I doubt this would be incompatible for anyone:

  • There are no known external implementations.

  • This never really behaved like a regular extension point to begin with: the user cannot select an implementation! In fact FilePathCopyMethod was dead code—even as of 3cf80e5, it was always overridden by FingerprintingCopyMethod. And that one was rather misnamed as of #11—it may or may not do fingerprinting!

  • JENKINS-7753 claimed as justification

    I would like to implement a plug-in which provides CopyArtifact with rsync capabilities when copying workspaces between jobs. This will decrease load on the master and most likely increase IO-throughput.

    which is sensible enough but better handled via ArtifactManager, especially as of JENKINS-49635.

I am also making an API change to BuildSelector. In this case there are two known external implementations but neither override the getSourceDirectory method. In fact, other than WorkspaceSelector, it is hard to imagine any other useful override. I am making a cursory effort to retain compatibility with any external overrides just in case.

Subsumes #98.

@reviewbybees

@jglick jglick requested review from oleg-nenashev and ikedam Feb 20, 2018

jglick added some commits Feb 21, 2018

@ikedam

ikedam approved these changes Feb 23, 2018

Copy link
Member

left a comment

Looks good to me.

you should create a tarball and archive that instead

+1
Users are recommended to use zip, unzip steps or call tar command via sh step.
Unfortunately, zip step doesn't support exclude filters...

*/
@Deprecated
public abstract class Copier implements ExtensionPoint {

This comment has been minimized.

Copy link
@ikedam

ikedam Feb 23, 2018

Member

We can remove them.
It's useless to preserving binary compatibilities as they're completely no longer used,
and exceptions should occur if someone use them.

This comment has been minimized.

Copy link
@jglick

jglick Feb 24, 2018

Author Member

Well, no “exception” would result from using them somehow. I do not have a strong opinion either way.

@@ -412,7 +426,7 @@ public void perform(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonn
throw new AbortException(message);
}
}
FilePath targetDir = workspace, baseTargetDir = targetDir;

This comment has been minimized.

Copy link
@ikedam

ikedam Feb 23, 2018

Member

Note for me: baseTargerDir looks not used at all. Completely useless.

This comment has been minimized.

Copy link
@jglick

jglick Feb 24, 2018

Author Member

Right, AFAICT it was never used anyway.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2018

@ikedam so I am leaving it up to your judgment what to do here. I noted the number of JIRA votes for various affected issues, to the extent that is meaningful data. This PR in isolation implements a feature with 14 votes (and no good workaround), and regresses three bug reports with a combined total of 14 votes (and the workaround of archiving a tarball rather than individual files). #100 implements the feature (and more) without regressing any of the prior bugs, but will require a new core baseline. 🤷‍♂

One option not previously discussed would be to merge this but release only as a -beta version, allowing users interested in JENKINS-22637 to get the implementation from the experimental update center; and then release #100 if and when the corresponding core PR is released (the update center is smart enough not to offer that update to anyone using older LTS lines).

The hope is that once JEP-301 is deployed, Essentials users would get new stuff that just works, right away. Obviously a lot needs to happen before that is a reality.

@oleg-nenashev
Copy link
Member

left a comment

Looks good in general. Some code could be moved to the core or API plugin if there are use-case in other plugins. 🐝

for (FilePath file : list)
copier.copyOne(file, new FilePath(targetDir, file.getName()), isFingerprintArtifacts());
cnt = list.length;
targetDir.mkdirs(); // Create target if needed

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

would be nice to switch it to Java7 Files API while you are around

try {
md5 = MessageDigest.getInstance("MD5");
} catch (NoSuchAlgorithmException x) {
throw new AssertionError(x);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

does not really matter, but maybe just IOException? There were some discussions about disabling MD5 at all to harden the security. I'd guess it is not realistic, but I would rather prefer to avoid errors in the production code

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

To do that we would first need to reimplement fingerprints in Jenkins core, which would surely force changes here as well anyway.

}
}

private static List<String> scan(VirtualFile root, String expandedFilter, @CheckForNull String expandedExcludes) throws IOException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Maybe it should be moved to core utils eventually. Looks to be generic enough

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

This code is deleted in #100.

targetDir.mkdirs(); // Create target if needed
List<String> list = scan(srcDir, expandedFilter, expandedExcludes);
for (String file : list) {
copyOne(src, dst, fingerprints, srcDir.child(file), new FilePath(targetDir, isFlatten() ? file.replaceFirst(".+/", "") : file), md5);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

passing isFingerprintArtifacts() looked better, but OK

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

But now we need to pass in the map.

copier.end();
if (fingerprints != null) {
for (Run<?, ?> r : new Run<?, ?>[] {src, dst}) {
if (fingerprints.size() > 0) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

It is not modified inside the cycle IIUC, maybe makes sense to just move to the outer check

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

Does not really matter—the loop has only two iterations.

return pattern;
}

private static void copyOne(Run<?,?> src, Run<?,?> dst, Map<String, String> fingerprints, VirtualFile s, FilePath d, MessageDigest md5) throws IOException, InterruptedException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

same, does not seem to be plugin-specific

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

It is pretty specific to copyartifact I think.

targetDir.mkdirs(); // Create target if needed
List<String> list = scan(srcDir, expandedFilter, expandedExcludes);
for (String file : list) {
copyOne(src, dst, fingerprints, srcDir.child(file), new FilePath(targetDir, isFlatten() ? file.replaceFirst(".+/", "") : file), md5);

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 14, 2018

Member

Note for me: snipping ".+/" is equivalent to the current implementation calling FilePath#getName() in FingerprintingCopyMethod.
(I'm not sure this is a good behavior as reported in JENKINS-16987)

This comment has been minimized.

Copy link
@jglick

jglick Mar 14, 2018

Author Member

No, this replaceFirst call just implements the flatten mode. It has nothing to do with fingerprinting. The behavior you mention is retained in copyOne as

fingerprints.put(s.getName(), digest);
//                ^^^^^^^^^^

This comment has been minimized.

@ikedam

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@jglick Sorry for my late response.

  • Releasing to the experimental update center at first
  • Releasing to the actual update center when jenkinsci/jenkins#3302 is released

This option looks good to me.

Let me have a little more time to work for https://issues.jenkins-ci.org/browse/JENKINS-47905 and https://issues.jenkins-ci.org/browse/JENKINS-47074.
So releases:

  • copyartifact-1.40: JENKINS-47905 , JENKINS-47074
  • copyartifact-1.41-beta: JENKINS-22637
  • copyartifact-1.41: JENKINS-22637

@jglick jglick changed the title [JENKINS-22637] ArtifactManager support [on-hold] [JENKINS-22637] ArtifactManager support Mar 14, 2018

@ikedam

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

As JENKINS-47074 will be resolved as Won't fix and JENKINS-47905 provides no new features, versions are:

copyartifact-1.39.1: JENKINS-47905
copyartifact-1.40-beta: JENKINS-22637
copyartifact-1.40: JENKINS-22637

@ikedam

This comment has been minimized.

Copy link
Member

commented Apr 29, 2018

Really sorry for long absence.
I resume this task.
Though the extension of VirtualFile is already released as Jenkins2.108, I believe it still make sense to release 1.40-beta for LTS users.

@ikedam ikedam changed the title [on-hold] [JENKINS-22637] ArtifactManager support [JENKINS-22637] ArtifactManager support Apr 29, 2018

@ikedam ikedam merged commit 1f762e5 into jenkinsci:master Apr 29, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@ikedam

This comment has been minimized.

Copy link
Member

commented Apr 29, 2018

Hmm... It seems that releasing 1.40 will mask 1.40-beta as described in https://jenkins.io/doc/developer/publishing/releasing-experimental-updates/.
I might no longer make sense to release 1.40-beta.

@ikedam

This comment has been minimized.

Copy link
Member

commented Apr 29, 2018

Released 1.40-beta-1 with this change.
I couldn't update the wiki page as login failure (looks like a system error). I'll try it later.

@jglick

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

releasing 1.40 will mask 1.40-beta

AFAIK not, since the UC for LTS users should only be offering 1.39.

@jglick jglick deleted the jglick:VirtualFile-JENKINS-22637 branch May 1, 2018

@ikedam

This comment has been minimized.

Copy link
Member

commented May 5, 2018

Updates center provides several versions of plugin lists for core versions by redirection.

But the experimental update center provides only one version, and will result 1.40-beta-1 inaccessible via the update center.

Anyway, that doesn't matter so much as users can download the beta version directly from http://updates.jenkins-ci.org/download/plugins/copyartifact/1.40-beta-1/copyartifact.hpi .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.