Skip to content

add getBlobStore method #85

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.

Sign up for GitHub

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

Closed
mawinter69 wants to merge 9 commits into from
Closed

add getBlobStore method #85

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
64cb425
add getBlobStore method
mawinter69 Nov 30, 2018
2614cfd
add default implementation for getBlobStore()
mawinter69 Dec 1, 2018
444c42f
more places where to replace getContext() with getBlobStore()
mawinter69 Dec 1, 2018
32e83ab
remove unneeded override
mawinter69 Dec 1, 2018
d27837b
cleanup imports
mawinter69 Dec 4, 2018
4b931eb
revert changes to files where only whitespace has changed
mawinter69 Mar 28, 2019
da40ea1
remove unused import
mawinter69 Mar 28, 2019
88916d4
Merge branch 'master' of https://github.com/jenkinsci/artifact-manage…
mawinter69 Sep 12, 2019
060e8e7
Merge branch 'master' of github.com:jenkinsci/artifact-manager-s3-plugin
mawinter69 Jan 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/main/java/io/jenkins/plugins/artifact_manager_jclouds/BlobStoreProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.net.URI;
import java.net.URL;

import org.jclouds.blobstore.BlobStore;
import org.jclouds.blobstore.BlobStoreContext;
import org.jclouds.blobstore.domain.Blob;
import org.kohsuke.accmod.Restricted;
Expand Down Expand Up @@ -69,6 +70,12 @@ public enum HttpMethod {
@NonNull
public abstract BlobStoreContext getContext() throws IOException;
Copy link
Member

@jglick jglick Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could perhaps be made nonabstract (throwing an AbstractMethodError at runtime if neither method is overridden, to avoid a StackOverflowError), since in the other direction there is a BlobStore.getContext() (used in this plugin only to close the context for tests…which is likely a bug).

Copy link
Author

@mawinter69 mawinter69 Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where BlobStore.getContext() is used. In the test BlobStoreProvider.getContext() is used and on this context the close() is called.
I think it should stay abstract.

Copy link
Member

@jglick jglick Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where BlobStore.getContext() is used.

Sorry, to clarify: I think we could write something along the lines of:

public /* final */ BlobStoreContext getContext() throws IOException {
    if (Util.isOverridden(BlobStoreProvider.class, getClass(), "getBlobStore")) {
        return getBlobStore().getContext();
    } else {
        throw new AbstractMethodError("override getBlobStore");
    }
}
public /* abstract */ BlobStore getBlobStore() throws IOException {
    return getContext().getBlobStore();
}

where the modifiers could be uncommented and the code simplified if we did not care to retain backward compatibility for hypothetical external implementations (none are known).

Copy link
Author

@mawinter69 mawinter69 Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this might work as well, when speaking of not retaining backward compatibility one could also consider removing the getContext() method completely.
The getBlobStore should always be abstract I think.

Copy link
Member

@jglick jglick Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when speaking of not retaining backward compatibility one could also consider removing the getContext() method completely

Yes, you could just inline it in that case.

The getBlobStore should always be abstract

Then calls to this method against a (hypothetical) existing external implementation, which overrode/implemented getContext, would fail with an AbstractMethodError.

The above paired method idiom is common in Jenkins sources, by the way, when we change our minds about what methods an implementation of some interface (or abstract class) should define. Somewhat cleaner is to just deprecate the whole “interface” and introduce, say, a BlobStoreProvider2 while having callers (anything enumerating this ExtensionList) convert from the old implementation.

Again, any of these tricks may just be overkill—the only other implementation of JEP-202 I know about is azure-artifact-manager, which uses the Azure APIs directly rather than via this plugin and jclouds.


/** Return the jclouds Blobstore for working with blob. */
@NonNull
public BlobStore getBlobStore() throws IOException {
return getContext().getBlobStore();
}

/**
* Get a provider-specific URI.
*
Expand Down
17 changes: 8 additions & 9 deletions src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
import jenkins.util.VirtualFile;
import org.apache.http.client.methods.HttpGet;
import org.jclouds.blobstore.BlobStore;
import org.jclouds.blobstore.BlobStoreContext;
import org.jclouds.blobstore.BlobStores;
import org.jclouds.blobstore.domain.Blob;
import org.jclouds.blobstore.domain.StorageMetadata;
Expand Down Expand Up @@ -116,7 +115,7 @@ public void archive(FilePath workspace, Launcher launcher, BuildListener listene
throws IOException, InterruptedException {
LOGGER.log(Level.FINE, "Archiving from {0}: {1}", new Object[] { workspace, artifacts });
Map<String, URL> artifactUrls = new HashMap<>();
BlobStore blobStore = getContext().getBlobStore();
BlobStore blobStore = getBlobStore();

// Map artifacts to urls for upload
for (Map.Entry<String, String> entry : artifacts.entrySet()) {
Expand Down Expand Up @@ -160,7 +159,7 @@ public boolean delete() throws IOException, InterruptedException {
LOGGER.log(Level.FINE, "Ignoring blob deletion: {0}", blobPath);
return false;
}
return JCloudsVirtualFile.delete(provider, getContext().getBlobStore(), blobPath);
return JCloudsVirtualFile.delete(provider, getBlobStore(), blobPath);
}

@Override
Expand All @@ -170,7 +169,7 @@ public VirtualFile root() {

@Override
public void stash(String name, FilePath workspace, Launcher launcher, EnvVars env, TaskListener listener, String includes, String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException {
BlobStore blobStore = getContext().getBlobStore();
BlobStore blobStore = getBlobStore();

// Map stash to url for upload
String path = getBlobPath("stashes/" + name + ".tgz");
Expand Down Expand Up @@ -233,7 +232,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt

@Override
public void unstash(String name, FilePath workspace, Launcher launcher, EnvVars env, TaskListener listener) throws IOException, InterruptedException {
BlobStore blobStore = getContext().getBlobStore();
BlobStore blobStore = getBlobStore();

// Map stash to url for download
String blobPath = getBlobPath("stashes/" + name + ".tgz");
Expand Down Expand Up @@ -279,7 +278,7 @@ public void clearAllStashes(TaskListener listener) throws IOException, Interrupt
return;
}

BlobStore blobStore = getContext().getBlobStore();
BlobStore blobStore = getBlobStore();
int count = 0;
try {
for (StorageMetadata sm : BlobStores.listAll(blobStore, provider.getContainer(), ListContainerOptions.Builder.prefix(stashPrefix).recursive())) {
Expand All @@ -303,7 +302,7 @@ public void copyAllArtifactsAndStashes(Run<?, ?> to, TaskListener listener) thro
}
JCloudsArtifactManager dest = (JCloudsArtifactManager) am;
String allPrefix = getBlobPath("");
BlobStore blobStore = getContext().getBlobStore();
BlobStore blobStore = getBlobStore();
int count = 0;
try {
for (StorageMetadata sm : BlobStores.listAll(blobStore, provider.getContainer(), ListContainerOptions.Builder.prefix(allPrefix).recursive())) {
Expand All @@ -320,8 +319,8 @@ public void copyAllArtifactsAndStashes(Run<?, ?> to, TaskListener listener) thro
listener.getLogger().printf("Copied %d artifact(s)/stash(es) from %s to %s%n", count, provider.toURI(provider.getContainer(), allPrefix), provider.toURI(provider.getContainer(), dest.getBlobPath("")));
}

private BlobStoreContext getContext() throws IOException {
return provider.getContext();
private BlobStore getBlobStore() throws IOException {
return provider.getBlobStore();
}

}
23 changes: 11 additions & 12 deletions src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsVirtualFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import java.util.stream.StreamSupport;

import org.jclouds.blobstore.BlobStore;
import org.jclouds.blobstore.BlobStoreContext;
import org.jclouds.blobstore.BlobStores;
import org.jclouds.blobstore.domain.Blob;
import org.jclouds.blobstore.domain.MutableBlobMetadata;
Expand Down Expand Up @@ -80,7 +79,7 @@ public class JCloudsVirtualFile extends VirtualFile {
@CheckForNull
private transient Blob blob;
@CheckForNull
private transient BlobStoreContext context;
private transient BlobStore blobStore;

public JCloudsVirtualFile(@NonNull BlobStoreProvider provider, @NonNull String container, @NonNull String key) {
this.provider = provider;
Expand All @@ -93,18 +92,18 @@ public JCloudsVirtualFile(@NonNull BlobStoreProvider provider, @NonNull String c

private JCloudsVirtualFile(@NonNull JCloudsVirtualFile related, @NonNull String key) {
this(related.provider, related.container, key);
context = related.context;
blobStore = related.blobStore;
}

/**
* Build jclouds blob context that is the base for all operations
* Build jclouds blobstore that is the base for all operations
*/
@Restricted(NoExternalUse.class) // testing only
BlobStoreContext getContext() throws IOException {
if (context == null) {
context = provider.getContext();
BlobStore getBlobStore() throws IOException {
if (blobStore == null) {
blobStore = provider.getBlobStore();
}
return context;
return blobStore;
}

private String getContainer() {
Expand All @@ -129,9 +128,9 @@ public String getName() {
private Blob getBlob() throws IOException {
if (blob == null) {
LOGGER.log(Level.FINE, "checking for existence of blob {0} / {1}", new Object[] {container, key});
blob = getContext().getBlobStore().getBlob(getContainer(), getKey());
blob = getBlobStore().getBlob(getContainer(), getKey());
if (blob == null) {
blob = getContext().getBlobStore().blobBuilder(getKey()).build();
blob = getBlobStore().blobBuilder(getKey()).build();
blob.getMetadata().setContainer(getContainer());
}
}
Expand Down Expand Up @@ -164,7 +163,7 @@ public boolean isDirectory() throws IOException {
return frame.children.keySet().stream().anyMatch(f -> f.startsWith(relSlash));
}
LOGGER.log(Level.FINE, "checking directory status {0} / {1}", new Object[] {container, key});
return !getContext().getBlobStore().list(getContainer(), prefix(key + "/")).isEmpty();
return !getBlobStore().list(getContainer(), prefix(key + "/")).isEmpty();
}

@Override
Expand Down Expand Up @@ -196,7 +195,7 @@ private Iterable<StorageMetadata> listStorageMetadata(boolean recursive) throws
if (recursive) {
options.recursive();
}
return BlobStores.listAll(getContext().getBlobStore(), getContainer(), options);
return BlobStores.listAll(getBlobStore(), getContainer(), options);
}

@Override
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.jclouds.ContextBuilder;
import org.jclouds.aws.domain.SessionCredentials;
import org.jclouds.aws.s3.AWSS3ProviderMetadata;
import org.jclouds.blobstore.BlobStore;
import org.jclouds.blobstore.BlobStoreContext;
import org.jclouds.blobstore.domain.Blob;
import org.jclouds.domain.Credentials;
Expand Down Expand Up @@ -125,7 +126,7 @@ public BlobStoreContext getContext() throws IOException {
throw new IOException(x);
}
}

Copy link
Member

@jglick jglick Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to revert changes to this file.

/**
* field only for tests.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public synchronized BlobStoreContext getContext() throws IOException {
}
return context;
}

Copy link
Member

@jglick jglick Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@Override
public URI toURI(String container, String key) {
return URI.create("mock://" + container + "/" + key);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public class NetworkTest {
@Before
public void configureManager() throws Exception {
MockBlobStore mockBlobStore = new MockBlobStore();
mockBlobStore.getContext().getBlobStore().createContainerInLocation(null, mockBlobStore.getContainer());
mockBlobStore.getBlobStore().createContainerInLocation(null, mockBlobStore.getContainer());
ArtifactManagerConfiguration.get().getArtifactManagerFactories().add(new JCloudsArtifactManagerFactory(mockBlobStore));
}

Expand Down
5 changes: 5 additions & 0 deletions src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jvnet.hudson.test.Issue;
import org.jclouds.blobstore.BlobStore;
import org.jclouds.blobstore.BlobStoreContext;
import org.jclouds.blobstore.domain.Blob;
import org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProject;
Expand Down Expand Up @@ -142,6 +143,10 @@ public BlobStoreContext getContext() throws IOException {
return delegate.getContext();
}
@Override
public BlobStore getBlobStore() throws IOException {
return delegate.getBlobStore();
}
@Override
public URI toURI(String container, String key) {
return delegate.toURI(container, key);
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3AbstractTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public void setupContext() throws Exception {

context = provider.getContext();

blobStore = context.getBlobStore();
blobStore = provider.getBlobStore();

setup();
}
Expand Down