Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[EP] Remote Artifacts #419
[EP] Remote Artifacts #419
Changes from 1 commit
55e8181
5c2dcee
a112672
6f95ad8
02020e8
01ace39
a34984a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe consider extending the source section instead ?
This would be an extensible mechanism. In the future we might support other types (loading from maybe some configmap where the cli placed the local sources).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can introduce glue-code to autodetect the type based on the url, keeping the whole
spec.source
as simple as it is now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be hard. An HTTPS URL can be a public Git repository (especially as we do not require the
.git
suffix at the moment), a SubVersion repository, a plain file and probably more.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given the distinction for types of data, one being VCS and the other remote artifacts, I think they should be described in different logical blocks. So, we also have a clear separation between those two concerts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@otaviof a VCS resource is also remote. There is "just" a different protocol to download them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigMap
is certainly a valid use case, though with a cli upload I see PersistentVolume/PVC being a more likely result.Another thing we should clarify is that "sources" are things that are intended to be present in the resulting image. There is a separate use case for "volume" support (like caches), in which case the data present in the volume is not included in the resulting container image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want to employ the discriminator pattern?
(note that
type: Git
may not be required, apimachinery can fill it in).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same line of thought, I see a VCS as yet another remote artifact. I´m not able to understand the distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider latest commit, I'm adding a initial
.spec.sources
support with your inputs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@otaviof thanks, I took a look.
Is the new field from above required? we already have something call
contextDir
, can this serve the same purpose?@adambkaplan @SaschaSchwarze0 @otaviof I have some concerns around
spec.sources
. First of all I think introducing atype
is a nice way to distinguish the different assets(remote files, git,etc). I´m struggling to understand the value of moving from of a map(spec.source) into a list(spec.sources). My rational is:spec.source
is for me one of the structs that is very simple to understand, and I would like to keep it this way. Adding support for a list of sources is going to add complexity on that API level.spec.sources
will introduce some code complexity when generating the Taskrun. It would be good to highlight those and decide if it worth the effort.@sbose78 probably we want your feedback also. Also, do we have some good references on how Build v1 did it(shouldnt bias us)?, but it will help to understand the rational there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be placed in an "Alternatives" section at the bottom of the proposal. I believe we decided that we are not going to pursue this approach.
Leaving this in the main body confuses what is in the proposal vs. out. See the proposal template for guidance:
https://github.com/shipwright-io/build/blob/master/docs/proposals/guidelines/proposal-template.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the template, the standalone CRD alternative design was moved into a section at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "download" looks critical. We get this for free from tekton, Wouldn´t it make more sense to do this natively with "golang" or to see if Tekton have already something for us? e.g. they do not use a "git" binary for pulling, they have their own git pkg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what Tekton does for the VCS (source-code), but what would it do for remote artifacts? Could we use Tekton to download them as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally there would be a Tekton resource handling things like plain HTTP(S) download. But, I think there is not one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not appear such a resource type exists. If we accept this proposal, I recommend submitting a TEP to see if we can get upstream Tekton buy-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mainly referring to do a native implementation in go, I would like to avoid introducing extra binaries dependencies in the code. We might not need Tekton at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we all agree this is not enforcing the usage of
wget
and that another more native implementation(e.g. tekton way) might be reused. Can we mention this somewhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leverage the Tekton source to enable storage or something first?:
https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#resource-types
I think
wget
is a little weak. How about the private resource (with auth) or ftp?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PipelineResources
at this time are most likely not going to get out of alpha stage. There has been a fair amount of consternation and churn around how exactly how to move forward, though.But minimally I would not advise a
PipelineResource
styled path.That said, if I had to pick, I'd say "custom tasks" might be the most likely path along the "tekton resource" angle. Also, submitting something "similar" but different most likely would get the response of "look at custom tasks".
See https://github.com/tektoncd/community/blob/master/teps/0002-custom-tasks.md for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also a "specializing task" which has some literature around it but no TEP as of yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a specific resource in upstream Tekton would be a hard sell at the moment given the direction
PipelineResources
is taking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to see us use pure golang for the downloading instead of an external tool such as wget, i don't like having to rely on os provided tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like idea from Corey. We will eventually face a Build which has a longer list of sources. For all these sources beside the git source (which we will probably want to continue to handle using Tekton's Git resource), I envision one Task step implemented by ourselves using Golang. Within our code we can then do optimizations like parallel downloads for different sources and also can handle and report errors in a better way than by invoking an external command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like the idea of creating or own tool for that purpose. However, I see
wget
as a simple starting point, on which we could potentially develop our own tool in parallel.So, how about get started with
wget
and evaluate the new tooling on its own track?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised the same concern on some comments ago, I do not see why we need to rely on a binary. #419 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can agree that we can implement the downloader, regardless of whether we build our own in go vs. "buy" it (by using a fixed container image with
wget
). IMO deciding which approach to take is beyond what is necessary to mark this EP as "implementable".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buildrun controller needs to generate a TaskRun step that would generate the
wget
command to download the artifacts specifiedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a good addition. Please consider latest commit
01ace39
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@otaviof pls correct me if I´m wrong, but here the example is based on the assumption that we support a new
BuildSource
CRD. If we do not have theBuildSource
CRD in place, then we will still need to havespec.sources
for each logo typeis the above correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct @qu1queee. The example was made to illustrate both scenarios at once, but it might have been too much leaning towards the "Alternative" scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks