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.

Sign up for GitHub

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

Jump to bottom

Remove redundant label added to Pods #494

Merged
merged 1 commit into from
Feb 12, 2019
Merged

Remove redundant label added to Pods #494

merged 1 commit into from
Feb 12, 2019

Conversation

dwnusbaum
Copy link
Contributor

@dwnusbaum dwnusbaum commented Feb 12, 2019 •
edited
Loading

Follow-up to review comments from @shashwathi in #488, mostly in reference to #488 (comment).

Specifically, this PR removes the build.knative.dev/buildName label since the pipeline.knative.dev/taskRun label has the same value and the former label is somewhat confusing since we never actually create a Build.

Still working on e2e tests for labels, I can add them here once I have them ready or in another follow-up if desired.

@dwnusbaum
Remove redundant label added to Pods
22ddfef 
Previously, we added a label with key "build.knative.dev/buildName"
to Pods. That label was redundant with a different label whose key
is "pipeline.knative.dev/taskRun" as of beda1f8. Since we are no
longer depending on Knative Build at runtime, it seems best to
remove the redundant label.

Follow-up to tektoncd#488.
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 12, 2019
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 12, 2019
@dwnusbaum dwnusbaum mentioned this pull request Feb 12, 2019
Merged
@shashwathi
Copy link
Contributor

/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 Feb 12, 2019
@shashwathi
Copy link
Contributor

/assign @shashwathi

@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/resources/pod.go 83.3% 83.6% 0.3

@abayer
Copy link
Contributor

abayer commented Feb 12, 2019

...I'm going to hold of on lgtming this time until I'm sure it's actually done. =)

@shashwathi
Copy link
Contributor

Thanks @abayer . Appreciate that

@shashwathi
Copy link
Contributor

@dwnusbaum : If you would like to follow up with another PR for e2e test with labels then I am okay with merging this PR as is. This PR is pretty contained with cleaning up build label.

shashwathi
shashwathi reviewed Feb 12, 2019
View reviewed changes
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.

Thanks for following up 👍 appreciate it

@dwnusbaum
Copy link
Contributor Author

If you would like to follow up with another PR for e2e test with labels then I am okay with merging this PR as is. This PR is pretty contained with cleaning up build label.

@shashwathi Sounds good to me, I will follow up with the e2e tests in another PR

shashwathi
shashwathi approved these changes Feb 12, 2019
View reviewed changes
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.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dwnusbaum, shashwathi

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:

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 Feb 12, 2019
@knative-prow-robot knative-prow-robot merged commit b52f7a9 into tektoncd:master Feb 12, 2019
@dwnusbaum dwnusbaum deleted the 458-propagate-labels-to-pods branch February 12, 2019 20:48
@bobcatfish
Copy link
Collaborator

nice cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shashwathi shashwathi shashwathi approved these changes

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dwnusbaum @shashwathi @knative-metrics-robot @abayer @knative-prow-robot @bobcatfish @googlebot