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

Add labels for tracking objects lineage #199

Merged

Conversation

tanner-bruce
Copy link

Adds labels to TaskRuns created by PipelineRuns as well as Builds that
are created. In TaskRuns and Builds will have labels set telling which
Pipeline, PipelineRun and TaskRun are the owners of it.

This is useful when doing filtering in searches.

Fixes #69

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 29, 2018
@nader-ziada
Copy link
Member

/ok-to-test

@knative-prow-robot knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 29, 2018
Copy link
Member

@nader-ziada nader-ziada left a comment

Choose a reason for hiding this comment

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

  • we should add an integration test to check the propagation of labels from pipeline to taskrun
  • start a spec document showing what labels appear on which objects, an example from serving

pkg/reconciler/v1alpha1/taskrun/taskrun.go Outdated Show resolved Hide resolved
pkg/reconciler/v1alpha1/taskrun/taskrun.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shashwathi shashwathi left a comment

Choose a reason for hiding this comment

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

Couple of suggestions.

  1. Along the line of @pivotal-nader-ziada suggestion, consider declaring labels from pipelinerun, taskrun into a constants file. For example in serving component register.go holds these labels.
    Ref: https://github.com/knative/serving/blob/master/pkg/apis/serving/register.go
  2. I would recommend the label to have prefix build-pipeline.knative.dev/componentName to be consistent with other components.

@tanner-bruce
Copy link
Author

tanner-bruce commented Oct 29, 2018

@pivotal-nader-ziada

we should add an integration test to check the propagation of labels from pipeline to taskrun

I made the TestPipelineRun test verify that the TaskRun has the proper labels set. Do you think this sufficient?

@nader-ziada
Copy link
Member

thanks for the changes @tanner-bruce

/approve

@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, tanner-bruce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [pivotal-nader-ziada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2018
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

This looks great to me!

My only request is can we add something to our docs about this functionality? I'm thinking that using.md might be the best place, we could either:

  • Add a troubleshooting section
  • Add a "How do I find my objects" or "How do I find my Runs" section or something similar

I'm basically looking for something that just shows users they can use kubectl commands with these labels to find created objects.

(I'll leave it to @shashwathi or @pivotal-nader-ziada to add the lgtm when they're ready cuz they've been more involved in this review)

t.Errorf("Expected TaskRun %s to have succeeded but Status is %s", runName, c.Status)
}

if r.Labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] != hwPipelineRunName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

whoa, TIL you get the zero value of a type back if it doesn't exist in the map!

@nader-ziada
Copy link
Member

+1 for adding some kind of doc showing what labels are available on which objects, similar to the example from serving I mentioned earlier, but maybe add a section in using.md

@tanner-bruce
Copy link
Author

Conflicts are fixed and docs added.

docs/using.md Outdated Show resolved Hide resolved
```shell
kubectl get builds --all-namespaces -l pipeline.knative.dev/pipeline=build-image
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

excelleeeeeent thanks for adding these docs!

@bobcatfish
Copy link
Collaborator

@knative-prow-robot pull-knative-build-pipeline-integration-tests — Job failed.

wat

is this just a flake? maybe we should look into it :S

@bobcatfish
Copy link
Collaborator

/test pull-knative-build-pipeline-integration-tests

@bobcatfish
Copy link
Collaborator

/lgtm

Remove if you are ready:
/hold

@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 31, 2018
Adds labels to TaskRuns created by PipelineRuns as well as Builds that
are created. In TaskRuns and Builds will have labels set telling which
Pipeline, PipelineRun and TaskRun are the owners of it.

This is useful when doing filtering in searches.

Fixes tektoncd#69
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/taskrun/taskrun.go 76.5% 75.7% -0.8

@nader-ziada
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@nader-ziada
Copy link
Member

/hold cancel

not sure why there is a hold still

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2018
@knative-prow-robot knative-prow-robot merged commit e73d3af into tektoncd:master Nov 1, 2018
sthaha pushed a commit to sthaha/build-pipeline that referenced this pull request Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants