Skip to content
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

copy the labels from Task ->TaskRun to the generated Pod #458

Closed
jstrachan opened this issue Jan 31, 2019 · 10 comments · Fixed by #488
Closed

copy the labels from Task ->TaskRun to the generated Pod #458

jstrachan opened this issue Jan 31, 2019 · 10 comments · Fixed by #488

Comments

@jstrachan
Copy link

jstrachan commented Jan 31, 2019

Expected Behavior

If we add labels to a Task they should get replicated to the TaskRun then the Pod thats created so that you can more easily query/filter TaskRuns and Pods.

Actual Behavior

Labels on a Task are ignored

If there are concerns over copying the labels, maybe there could be a way to specify which labels to copy/not copy?

@abayer
Copy link
Contributor

abayer commented Jan 31, 2019

I'm going to start working on this shortly, fyi.

@vdemeester
Copy link
Member

/assign @abayer

@jstrachan jstrachan changed the title copy the labels from PipelineRun to the generated Task Pod copy the labels from TaskRun to the generated Pod Jan 31, 2019
@jstrachan jstrachan changed the title copy the labels from TaskRun to the generated Pod copy the labels from Task ->TaskRun to the generated Pod Jan 31, 2019
dwnusbaum added a commit to dwnusbaum/pipeline that referenced this issue Feb 11, 2019
Previously, labels were propagated from TaskRun to Build, but not from
Build to Pod. This PR fixes that, and also propagates labels from
PipelineRun to TaskRun, so that users are more easily able to filter,
identify, and query Pods created by Build Pipeline.

In all cases, if a user specifies a label whose key overlaps with one
of the labels that we set automatically, the user's label is ignored.

As mentioned in taskrun_test.go, the current tests do not fully
demonstrate that the TaskRun to Pod propagation works correctly since
they use Build as an intermediate form for comparison.

Fixes tektoncd#458
dwnusbaum added a commit to dwnusbaum/pipeline that referenced this issue Feb 11, 2019
Previously, labels were propagated from TaskRun to Build, but not from
Build to Pod. This PR fixes that, and also propagates labels from
PipelineRun to TaskRun, so that users are more easily able to filter,
identify, and query Pods created by Build Pipeline.

In all cases, if a user specifies a label whose key overlaps with one
of the labels that we set automatically, the user's label is ignored.

As mentioned in taskrun_test.go, the current tests do not fully
demonstrate that the TaskRun to Pod propagation works correctly since
they use Build as an intermediate form for comparison.

Fixes tektoncd#458
@dwnusbaum
Copy link
Contributor

I'm actually picking this up from Andrew :) Looking at the code, propagation from PipelineRun to TaskRun to Pod (through Build in memory) is pretty trivial, but propagation from Task through to Pod is a little trickier because right now most of the relevant data structures only pass a TaskSpec around but we'd need to full Task or at least its metadata. I discussed with @jstrachan and he was ok with leaving out Task for now if it requires significant extra work, but noted that it would be ideal to be able to use a single label to obtain all resources related to a build.

knative-prow-robot pushed a commit that referenced this issue Feb 12, 2019
Previously, labels were propagated from TaskRun to Build, but not from
Build to Pod. This PR fixes that, and also propagates labels from
PipelineRun to TaskRun, so that users are more easily able to filter,
identify, and query Pods created by Build Pipeline.

In all cases, if a user specifies a label whose key overlaps with one
of the labels that we set automatically, the user's label is ignored.

As mentioned in taskrun_test.go, the current tests do not fully
demonstrate that the TaskRun to Pod propagation works correctly since
they use Build as an intermediate form for comparison.

Fixes #458
@bobcatfish
Copy link
Collaborator

It feels kinda like the chain should go all the way:

  • Pipeline -> PipelineRun -> TaskRun -> pod
  • Task -> TaskRun -> pod

What do you think?

I'm actually picking this up from Andrew :)

Hooray!! 🎉 :D

most of the relevant data structures only pass a TaskSpec around but we'd need to full Task or at least its metadata.

hm good point - I'm sure we can figure out a way to make it work :D in the worse case we could always use the Task client to retrieve the Task again 🤔 - but also adding the labels to the resolved task resources or similar might not be too bad?

but noted that it would be ideal to be able to use a single label to obtain all resources related to a build.

@dwnusbaum relevant note, @tanner-bruce actually added a bit of label propagation in #199 (see the docs here which for my sins I deleted in #460 because I couldn't figure out where to put them 😇 (not a very good excuse)

@bobcatfish
Copy link
Collaborator

lol totally late to the party and the PR, sorry @dwnusbaum 🤣

@bobcatfish
Copy link
Collaborator

I am really tempted to put in a last request to add some docs on this but considering that I deleted the last ones I wouldn't blame you if you ignored me @dwnusbaum 😅 😅 😅

(in retrospect I should not have deleted them...)

@dwnusbaum
Copy link
Contributor

dwnusbaum commented Feb 12, 2019

It feels kinda like the chain should go all the way

I agree, it was just really easy to add the *Run propagation so I went for that first. Do you think it makes sense to open another issue specifically for Pipeline and Task label propagation, or just reopen this one? (I don't have permission to update the title/description so it might be cleaner to use a new issue)

but also adding the labels to the resolved task resources or similar might not be too bad?

That was what I was thinking too, but it seemed like at that point we might as well just have the whole Task in case anything needed additional metadata in the future (although maybe YAGNI...). From my quick look it seemed like there was no particular reason that we dropped the Task and just carried around the TaskSpec, so it would just be a matter of tweaking the struct and fixing up the use sites, but maybe I was missing something. Definitely deserves a closer look!

I am really tempted to put in a last request to add some docs on this

That seems fair to me 😄 I will try to whip something up from what was added in #199 and explain that custom labels are supported in addition to the labels we add to everything automatically. I am working on updating the e2e tests to check label propagation as well so I'll probably submit a PR for all of that.

@dwnusbaum
Copy link
Contributor

e2e tests updated and docs added in #500

@bobcatfish
Copy link
Collaborator

e2e tests updated and docs added in #500

niiiiiice 🙏 😍

Do you think it makes sense to open another issue specifically for Pipeline and Task label propagation, or just reopen this one?

A new issue is probably more satisfying too! :D

@dwnusbaum
Copy link
Contributor

A new issue is probably more satisfying too! :D

Created #501 😄

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 a pull request may close this issue.

5 participants