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

[JEP-202] Extending VirtualFile #3302

Merged
merged 31 commits into from Apr 20, 2018

Conversation

5 participants
@jglick
Member

jglick commented Feb 19, 2018

See JENKINS-49635 and also JENKINS-26810, and JEP-202.

Reference implementation is in the artifact-manager-s3 plugin.

@reviewbybees @jenkinsci/code-reviewers

@jglick jglick requested review from carlossg and oleg-nenashev Feb 19, 2018

@daniel-beck daniel-beck self-requested a review Feb 19, 2018

@jglick jglick referenced this pull request Feb 20, 2018

Merged

[JENKINS-22637] ArtifactManager support #99

3 of 7 tasks complete

@jglick jglick changed the title from [JENKINS-49635] Defining new VirtualFile methods to better support external artifact storage to [JENKINS-49635] [JENKINS-26810] Extending VirtualFile API Feb 21, 2018

@jglick jglick changed the title from [JENKINS-49635] [JENKINS-26810] Extending VirtualFile API to [JENKINS-49635] [JENKINS-26810] Extending VirtualFile Feb 21, 2018

jglick added some commits Feb 21, 2018

More compatible default implementation of VirtualFile.list(String, St…
…ring, boolean) which is also more convenient for some implementations.
SelectorUtils.tokenizePath apparently expects the native path separat…
…or, which is normally handled deep within DirectoryScanner.
@@ -1403,7 +1403,7 @@ public static File resolveSymlinkToFile(@Nonnull File link) throws InterruptedEx
* The relative path is meant to be resolved from the location of the symlink.
*/
@CheckForNull
public static String resolveSymlink(@Nonnull File link) throws InterruptedException, IOException {
public static String resolveSymlink(@Nonnull File link) throws IOException {

This comment has been minimized.

@jglick

jglick Feb 21, 2018

Member

Binary compatible simplification—the body never actually threw that exception.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Not source compatible IIRC (javac fails when there is a catch block for impossible exception), may impact PCT. There are usages of it externally, e.g. in the Support Core plugin: https://github.com/search?p=1&q=org%3Ajenkinsci+resolveSymlink&type=Code

I would propose to detach it to a separate PR if you feel it's important enough

This comment has been minimized.

@daniel-beck

daniel-beck Mar 14, 2018

Member

@oleg-nenashev Jesse already disputed this in #3340 (comment)

This comment has been minimized.

@oleg-nenashev

This comment has been minimized.

@jglick

jglick Mar 14, 2018

Member

Right, PCT should be unaffected, though it could impede

buildPlugin(jenkinsVersions: [null, '2.222']) // or whatever

Why the Java language treats an exception which cannot be thrown as a fatal error rather than a warning (ditto unreachable statements etc.), I do not know.

This comment has been minimized.

@jglick

jglick Mar 15, 2018

Member

The simple workaround for the buildPlugin case would be to just catch (Exception I guess.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 2, 2018

Member

would be better to fix it tho

@@ -355,7 +366,7 @@ private static String createBackRef(int times) {
private static void zip(OutputStream outputStream, VirtualFile dir, String glob) throws IOException {
try (ZipOutputStream zos = new ZipOutputStream(outputStream)) {
zos.setEncoding(System.getProperty("file.encoding")); // TODO JENKINS-20663 make this overridable via query parameter
for (String n : dir.list(glob.length() == 0 ? "**" : glob)) {
for (String n : dir.list(glob.isEmpty() ? "**" : glob, null, /* TODO what is the user expectation? */true)) {

This comment has been minimized.

@jglick

jglick Feb 21, 2018

Member

Retaining default excludes functionality here to maintain semantics but I am unsure whether users would want this or not.

}
return r.stream().filter(p -> {
TokenizedPath path = new TokenizedPath(p.replace('/', File.separatorChar));
return includePatterns.stream().anyMatch(patt -> patt.matchPath(path, true)) && !excludePatterns.stream().anyMatch(patt -> patt.matchPath(path, true));

This comment has been minimized.

@jglick

jglick Feb 21, 2018

Member

VirtualFileTest.Ram checks all this implementation.

* <p>Generally this will be harder to implement than {@link #asRemotable},
* which would have the opportunity to perform arbitrary preparation for {@link #open}
* such as negotiating session authentication.
* @return an externally usable URL like {@code https://gist.githubusercontent.com/ACCT/GISTID/raw/COMMITHASH/FILE}, or null if there is no such support

This comment has been minimized.

@jglick

jglick Feb 21, 2018

Member

Just the most familiar download URL example I could come up with—certainly not a choice for artifact storage!

This comment has been minimized.

@daniel-beck

daniel-beck Feb 21, 2018

Member

Whoever implements this as a weekend project will get 🍻 from me.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Adding some 🍺 to the trophy

@jglick jglick added on-hold and removed work-in-progress labels Feb 21, 2018

* <p>Since some implementations may in fact use external file storage,
* callers may request optional APIs to access those services more efficiently.
* Otherwise, for example, a plugin copying a file
* previously saved by {@link ArtifactManager} to an external storage servuce

This comment has been minimized.

@olivergondza

olivergondza Mar 3, 2018

Member

s/servuce/service/

jglick added some commits Mar 5, 2018

@oleg-nenashev

Looks good so far though I would still prefer InterruptedException. Accmod is still waiting for the release by @kohsuke

@@ -1403,7 +1403,7 @@ public static File resolveSymlinkToFile(@Nonnull File link) throws InterruptedEx
* The relative path is meant to be resolved from the location of the symlink.
*/
@CheckForNull
public static String resolveSymlink(@Nonnull File link) throws InterruptedException, IOException {
public static String resolveSymlink(@Nonnull File link) throws IOException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 2, 2018

Member

would be better to fix it tho

@@ -535,10 +547,10 @@ private int dirRank(VirtualFile f) {
* @param baseRef String like "../../../" that cancels the 'rest' portion. Can be "./"
*/
private static List<List<Path>> patternScan(VirtualFile baseDir, String pattern, String baseRef) throws IOException {
String[] files = baseDir.list(pattern);
Collection<String> files = baseDir.list(pattern, null, /* TODO what is the user expectation? */true);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 2, 2018

Member

Note that there was a JEP-200 regression in the similar code recently

This comment has been minimized.

@jglick

jglick Apr 3, 2018

Member

Yes, in DirScanner.Glob IIRC, which is called by some list implementations.

/** {@link Run.ArtifactList} without the implicit link to {@link Run} */
private static final class SerializableArtifactList extends ArrayList<SerializableArtifact> {
private static final long serialVersionUID = 1L;
private LinkedHashMap<SerializableArtifact, String> tree = new LinkedHashMap<>();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 2, 2018

Member

Even for a private class not looks a bit dangerous since you do not override modification methods to put data to both containers (array and tree)

This comment has been minimized.

@jglick

jglick Apr 3, 2018

Member

This code was already a mess and I am trying to touch it as little as possible. The problem here was that Artifact and ArtifactList were, surprisingly, nonstatic—they held a Run reference—which is unsupportable when they must be constructed inside a callable which might run remotely. (If using the FilePath-based VirtualFile, for example. In fact this is not currently used by any ArtifactManager, only workspace browsing, but the API does not preclude the possibility, so…)

* only on the node on which the object was created.
*
* <p>Since some implementations may in fact use external file storage,
* callers may request optional APIs to access those services more efficiently.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 2, 2018

Member

I still feel that we grow a technical debt in API, but Ican live with that.

throw (IOException) new InterruptedIOException().initCause(e);
target = Util.resolveSymlink(f);
} catch (IOException x) { // JENKINS-13202
target = null;

This comment has been minimized.

@oleg-nenashev

jglick added some commits Apr 2, 2018

@jglick jglick referenced this pull request Apr 2, 2018

Merged

asRemotable has been deleted #2

@carlossg

This comment has been minimized.

Contributor

carlossg commented Apr 13, 2018

this is stable now for final review

@carlossg

This comment has been minimized.

Contributor

carlossg commented Apr 13, 2018

@daniel-beck can you take a look at the new changes to see if your positive review still stands?

@Vlatombe Vlatombe self-requested a review Apr 16, 2018

@oleg-nenashev oleg-nenashev self-requested a review Apr 16, 2018

@carlossg

This is tested with the https://github.com/jenkinsci/artifact-manager-s3-plugin and works fine

@jglick jglick requested review from carlossg and oleg-nenashev and removed request for oleg-nenashev Apr 16, 2018

@oleg-nenashev

🐝 The diff from the previous review looks good to me. Since all APIs are annotated with Beta, we can reconsider APIs in the worst case

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 16, 2018

@jglick jglick changed the title from [JENKINS-49635] [JENKINS-26810] Extending VirtualFile to [JEP-202] Extending VirtualFile Apr 16, 2018

@carlossg

This comment has been minimized.

Contributor

carlossg commented Apr 20, 2018

Given the JEP is in place, no objections have been raised and all apis are marked as @beta I think this is ready to be merged for the benefit of downstream consumers, both the initial s3 implementation and any other interested implementers, and provide feedback

@jglick jglick added ready-for-merge and removed on-hold labels Apr 20, 2018

/**
* @deprecated use {@link #list(String, String, boolean)} instead
*/
@Deprecated

This comment has been minimized.

@daniel-beck

daniel-beck Apr 20, 2018

Member

This deprecation points to a Beta API. This seems unhelpful.

This comment has been minimized.

@jglick

jglick Apr 20, 2018

Member

Well, the only callers will be updated anyway.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Apr 20, 2018

FTR, agree with @carlossg regarding the eligibility for this to be merged. Beta annotations allow this to evolve if needed without breaking existing API commitments.

@jglick

This comment has been minimized.

Member

jglick commented Apr 20, 2018

@jglick jglick merged commit 51cac7d into jenkinsci:master Apr 20, 2018

1 check was pending

continuous-integration/jenkins/pr-merge This commit is being built
Details

@jglick jglick deleted the jglick:VirtualFile-JENKINS-49635 branch Apr 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment