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 all commits
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.
I am wondering if
sources
really makes sense here. I think of source as a git repository or something to that affect. Maybe this should be something more along the lines ofdependencies
like you would list in a maven file?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.
My vote is for
inputs
which would be a symmetric naming together with theoutput
image that we already have in the Build.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.
@SaschaSchwarze0 are you okay with
sources
and the path ahead to unify them later on?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 am okay with
sources
, yes. (But I doubt we would ever change this in the future. ;-) )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´m concerned more than one person is not fully buying the
spec.sources
, what are the implications for this EP to move intospec.inputs
rather thanspec.sources
, it will also keep the API clean for now. And in another EP we will deal with multiple gits, where we can tackle that modifications tospec.source
.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'm okay to use
.spec.inputs
for this purpose. Do we have objections on.spec.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.
not from my side :)
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.
@sbose78, @adambkaplan, @gabemontero, @coreydaley would you object to use
.spect.inputs
instead?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.
Could you use a more realistic example like a jar artifact in all examples, please?
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 initial text was using a Jar file, but it wind up in questions about the dependency management itself. So, moved that to a simpler thing, a license file.
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 note a more Java like example is kept here.
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 feel like name is superfluous here, what is the point of having it?
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 is so we can use the
merge
patch strategy. Otherwisekubectl apply
will default to overwriting everything in thesources
array.https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch
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 you expand upon this fields usage?
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'm moving the field to the new "Alternatives" 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.
This references the original CRD style of declaring a downloaded resource, which IIRC we are not pursuing. Please update.
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.
Moved to the new "Alternatives" section, please consider.
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.
Could you use a more realistic example like a jar artifact in all examples, please?
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 as in here.
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