-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduces Tekton bundles: take 2 #3142
Conversation
Hi @pierretasci. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
The following is the coverage report on the affected files.
|
usually force pushing a slightly modified commit works as well (not that that is a great solution either) |
The following is the coverage report on the affected files.
|
@pierretasci Haven't looked at the changes yet but from the PR policy perspective, we need to squash five commits into one 😞 and fix the This is very exciting and hoping to review it soon. |
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.
Lookin' good! Can't wait to see tkn support, especially, looking at the contract docs.
spec: | ||
taskRef: | ||
name: hello-world | ||
bundle: registry.hub.docker.com/ptasci67/test-oci@sha256:7a8e6b41d37e16c86dc38fbb395673fe496edb9ac759aee135df488b55b4c732 |
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 should move this to a Tekton-owned repo, maybe gcr.io/tekton-nightly?
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.
Absolutely! I don't think I have push access to that repo but I am happy to figure out how to do that or work with someone who does have push permissions.
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 what we'll probably end up doing is moving this to a real Go test that pushes a bundle image first then references it, to ensure it's there and up-to-date.
We can stick with this for now, but we should have a TODO to clean it up soon, in case Docker deletes the image or something 😅
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.
Will do.
// Check that the expected TaskRun was created | ||
actual := actions[1].(ktesting.CreateAction).GetObject().(*v1beta1.TaskRun) | ||
actual := actions[2].(ktesting.CreateAction).GetObject().(*v1beta1.TaskRun) |
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.
Is there some way we could loop and find the right action, instead of pulling out the one we expect to be right by index? This feels really brittle to implementation changes.
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.
As someone who had to change all these I am highly inclined to agree so to reduce the brittleness I added two helper functions!
funParam := tb.PipelineTaskParam("foo", "somethingfun") | ||
moreFunParam := tb.PipelineTaskParam("bar", "$(params.bar)") | ||
templatedParam := tb.PipelineTaskParam("templatedparam", "$(inputs.workspace.$(params.rev-param))") | ||
ps := tb.Pipeline("test-pipeline", |
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'd prefer not to add new usages of test builders if we can avoid it. I'm (slowly) trying to weed them out in favor of structs.
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.
Sure. Happy to change it though I am genuinely curious why we are moving away from builders. As the change shows, it is a bit tedious to write out the whole thing.
remotePipelines []runtime.Object | ||
ref *v1beta1.PipelineRef | ||
expected runtime.Object | ||
}{ |
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.
Nit: you can save some horizontal space by putting both {{s on the same line 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.
Done.
This is going to have so many merge conflicts with #3115 😅 But this is definitely higher priority than that, so let's get this in first and I'll clean up. |
dfa74e6
to
cb29b26
Compare
Much appreciated! Done. |
The following is the coverage report on the affected files.
|
cb29b26
to
2771338
Compare
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.
Addressed feedback from ImJasonH
spec: | ||
taskRef: | ||
name: hello-world | ||
bundle: registry.hub.docker.com/ptasci67/test-oci@sha256:7a8e6b41d37e16c86dc38fbb395673fe496edb9ac759aee135df488b55b4c732 |
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.
Absolutely! I don't think I have push access to that repo but I am happy to figure out how to do that or work with someone who does have push permissions.
remotePipelines []runtime.Object | ||
ref *v1beta1.PipelineRef | ||
expected runtime.Object | ||
}{ |
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.
Done.
// Check that the expected TaskRun was created | ||
actual := actions[1].(ktesting.CreateAction).GetObject().(*v1beta1.TaskRun) | ||
actual := actions[2].(ktesting.CreateAction).GetObject().(*v1beta1.TaskRun) |
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.
As someone who had to change all these I am highly inclined to agree so to reduce the brittleness I added two helper functions!
funParam := tb.PipelineTaskParam("foo", "somethingfun") | ||
moreFunParam := tb.PipelineTaskParam("bar", "$(params.bar)") | ||
templatedParam := tb.PipelineTaskParam("templatedparam", "$(inputs.workspace.$(params.rev-param))") | ||
ps := tb.Pipeline("test-pipeline", |
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.
Sure. Happy to change it though I am genuinely curious why we are moving away from builders. As the change shows, it is a bit tedious to write out the whole thing.
The following is the coverage report on the affected files.
|
2771338
to
fefc99c
Compare
The following is the coverage report on the affected files.
|
There's been some debate about their usefulness, especially compared with the effort it takes to hand-write the builder methods, keep them consistently named with consistent behavior, etc. -- by comparison, simply writing out the structs is natively supported already, and though it might end up being more keystrokes, I think it's actually more readable in the end. So now I'm on a crusade to wipe out test builders everywhere. 😅 |
docs/pipelines.md
Outdated
```yaml | ||
spec: | ||
tasks: | ||
- name: hello-world |
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.
nit: i think the - name: list can be unindented a bit.
docs/pipelines.md
Outdated
```yaml | ||
spec: | ||
tasks: | ||
- name: hello-world |
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 here: unindent.
docs/pipelines.md
Outdated
- name: hello-world | ||
taskRef: | ||
name: echo-task | ||
bundle: docker.com/myrepo/mycatalog@sha256:abc123 |
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 know this is just an example, but I think this should be "index.docker.io/myrepo/mycatalog..."
docs/taskruns.md
Outdated
spec: | ||
taskRef: | ||
name: echo-task | ||
bundle: docker.com/myrepo/mycatalog |
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 above - I think this needs to be index.docker.io for Dockerhub.
docs/taskruns.md
Outdated
`metadata.name` field of the `Task`. | ||
|
||
You may also specify a `tag` as you would with a Docker image which will give you a fixed, | ||
repeatable reference to a `Task`. |
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 the digest be the "fixed repeatable reference"?
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.
AFAIK, best practice is to treat tag
s as fixed elements but that is not an enforced aspect of the spec so I will call out digests as well
docs/taskruns.md
Outdated
of the same named `Task` to be run at once. | ||
|
||
`Tekton Bundles` may be constructed with any toolsets that produces valid OCI image artifacts so long as | ||
the artifact adheres to the [contract](tekton-bundle-contract.md). Additionally, you may also use the `tkn` |
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.
nit: the file is named "contracts", not "contract"
docs/tekton-bundle-contracts.md
Outdated
Each { `apiVersion`, `kind`, `title` } must be unique in the image. No resources of the | ||
same version and kind can be named the same. | ||
|
||
The contents of each layer must be the parsed YAML of the corresponding Tekton resource. If |
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.
Is the layer still a gzipped tar file, or just the raw YAML?
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.
Good point. This is a bit confusing as worded. The intention is that you should inject a single object into each layer in YAML/JSON form and then perform the standard compression/digesting required by the OCI spec. I will reword this to be more clear
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 looked at the code to see what actually happens, it might be worth specifying a bit more here. Each layer is a raw yaml file, we don't do any "tar-ing". That then gets compressed as it is stored in the registry.
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 this approach - but there are some cons to think through:
- Do we ever think we'll want to have multiple files per bundle? We can't do that without a tar.
- "docker pull/docker save" and "crane pull" expect image layers to be tars. The mime type is actually
application/vnd.oci.image.layer.v1.tar+gzip
- so clients might be expecting a tar here. We could change the mime type, but that might not be supported everywhere.
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.
Admittedly my knowledge of the underlying OCI standard is minimal but I will give it a go based on what I think I know. My understanding is that the go-containerregistry
tool before uploading the image, performs the necessary compression and tar'ing to make the image conform to the standard expected by the registry. If that is the case, I believe that means that while we store and read YAML bytes, transiently there is some compressing/decompressing happening.
Also, I am able to store multiple files in a bundle by adding each one as a new layer with the experimental tool I have built. I think that semi-validates my half-understanding that there is some compression happening in the background but I am happy to be wrong 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.
So go-containerregistry will compress, but not archive the layer. The mime type it sets by default is tar+gzip, but it's up to you to actually put these bytes into a tar and extract them from a tar when you call "Uncompressed".
I think we have two choices:
- Keep things the way they are (one object per layer, raw yaml bytes without tar-ing). Change the mime/media type of the layer. You can do this with a PR I just sent to go-containerregistry: google/go-containerregistry@071a121
- Tar files up when you put them in the layer, and keep the mime/media type the same. You can do this with https://golang.org/pkg/archive/tar/#NewReader
cc @jonjohnsonjr, the resident OCI spec expert.
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 excellent context that I didn't have so thanks for sharing! Based on your description, to me the latter option makes the most sense because it seems like a waste of space to store uncompressed files in a registry. I will add a note to the contract that the layers must be tar'ed.
fefc99c
to
005e946
Compare
KindAnnotation = "dev.tekton.image.kind" | ||
APIVersionAnnotation = "dev.tekton.image.apiVersion" | ||
TitleAnnotation = "dev.tekton.image.name" | ||
MaximumBundleObjects = 10 |
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.
Is it documented somewhere that we limit to 10 the content of a bundle ?
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.
Yup. It is spelled out in the contract: https://github.com/tektoncd/pipeline/pull/3142/files#diff-1dfbd644a9243c47edaba1835ff18e89bde7cb3b25b475356d5827d1bf8bbea9R20
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.
Ah right, make sense. We can change this later on if we need 👍
/lgtm |
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
@pierretasci @vdemeester looks like this has been failing with the same error:
|
c439a94
to
3ad7f1d
Compare
Yeah I am trying to figure out why this is failing in the GKE cluster but not locally. I think there is a timing issue happening here but I can't be sure. What I do know is that |
Worse yet, it always succeeds the first time but fails the second time. |
The following is the coverage report on the affected files.
|
/retest |
3ad7f1d
to
580e1fb
Compare
The following is the coverage report on the affected files.
|
580e1fb
to
6f4a37e
Compare
The following is the coverage report on the affected files.
|
6f4a37e
to
4aa3a64
Compare
The following is the coverage report on the affected files.
|
4aa3a64
to
c2be58e
Compare
The following is the coverage report on the affected files.
|
/test all |
The following is the coverage report on the affected files.
|
/lgtm |
/retest |
1 similar comment
/retest |
This regressed Tekton's ability to run e2e tests against clusters without the standard I opened #3429 to track. |
Changes
Introduces Tekton Bundles with docs, apis, examples, and updates to the reconcilers to allow Task and Pipeline references from Tekton Bundles
Note, I had to re-submit this PR to get the CLA to pass
Summary of changes:
This is a large change but there are significant side-effects to any one part of this change and it felt half-hearted to only provide the API change to either tasksruns or pipelines and not both at the same time.
Submitter Checklist
Release Notes