Skip to content

Conversation

@carlossg
Copy link
Contributor

about the reasoning and implementation

Address comments in #96

about the reasoning and implementation

Address comments in jenkinsci#96

We opted for the first, simpler option.

== Backwards Compatibility
Copy link
Member

Choose a reason for hiding this comment

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

eek, GitHub diff is getting confused, may trigger merge conflicts with #98 but OK

Currently this is a environment variable but could be a system property or a config section in the UI.

However:
This means that different runs can not store the artifacts in different buckets or paths, as we don't expect that to be a common use case.
Copy link
Member

Choose a reason for hiding this comment

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

cannot

Having access to the blob store would mean access to other jobs artifacts.

Agents only do URL based upload/download operations and get the correct url to do so from the master.
Extra care needs to be taken so agents do not have any access to the blob store.
Copy link
Member

Choose a reason for hiding this comment

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

Can delete this sentence now? Or shift to the next paragraph?

Copy link
Member

Choose a reason for hiding this comment

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

+1


http://javadoc.jenkins.io/hudson/FilePath.html[`FilePath`]

* `copyFromRemotely(URL)` Copies the content of a URL to a remote file without transferring content over a Remoting channel
Copy link
Member

Choose a reason for hiding this comment

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

doRemoteCopy might be a clearer name? BTW. I'm aware it's a change upstream, but as @jglick vented "it's still in Beta after all" :)

Copy link
Member

Choose a reason for hiding this comment

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

My main issue is that that would not obviously be a drop-in replacement for copyFrom(URL), as this is.



This change adds more methods to the VirtualFile API to support this flow.
This change adds more methods to the http://javadoc.jenkins.io/jenkins/util/VirtualFile.html[`VirtualFile`] API to support this flow.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

just minor comments



This change adds more methods to the VirtualFile API to support this flow.
This change adds more methods to the http://javadoc.jenkins.io/jenkins/util/VirtualFile.html[`VirtualFile`] API to support this flow.
Copy link
Member

Choose a reason for hiding this comment

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

Currently Javadoc site does not list the methods. Maybe makes sense to add return type and throws to have a full method signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the javadoc is now updated

Having access to the blob store would mean access to other jobs artifacts.

Agents only do URL based upload/download operations and get the correct url to do so from the master.
Extra care needs to be taken so agents do not have any access to the blob store.
Copy link
Member

Choose a reason for hiding this comment

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

+1

@oleg-nenashev
Copy link
Member

@carlossg do you plan to address the comments?

@bitwiseman bitwiseman merged commit df577eb into jenkinsci:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants