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

In 'kubectl describe', find controllers with ControllerRef, instead of showing the original creator #42849

Merged
merged 1 commit into from
Apr 12, 2017
Merged

In 'kubectl describe', find controllers with ControllerRef, instead of showing the original creator #42849

merged 1 commit into from
Apr 12, 2017

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented Mar 9, 2017

@enisoc @Kargakis @kubernetes/sig-apps-pr-reviews @kubernetes/sig-cli-pr-reviews

In 'kubectl describe', find controllers with ControllerRef, instead of showing the original creator.

@janetkuo janetkuo added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 9, 2017
@janetkuo janetkuo added this to the v1.6 milestone Mar 9, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 9, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 9, 2017
@janetkuo janetkuo force-pushed the kubectl-describe-controllerRef branch from 96b7149 to dd7e2d8 Compare March 10, 2017 00:15
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 10, 2017
@smarterclayton
Copy link
Contributor

This is really late for 1.6 - do we really have to do this?

@janetkuo
Copy link
Member Author

janetkuo commented Mar 10, 2017 •
edited
Loading

@smarterclayton I don't see this as release blocking, but current kubectl describe currently shows the creator as Controller, not the actually controller from ControllerRef. This may cause user confusion.

@janetkuo janetkuo force-pushed the kubectl-describe-controllerRef branch from dd7e2d8 to 57703ae Compare March 10, 2017 01:12
0xmichalis
0xmichalis reviewed Mar 10, 2017
View reviewed changes
pkg/printers/internalversion/describe.go Outdated
w.Write(LEVEL_0, "Created By:\t%s\n", createdBy)
}
if controlledBy := printController(pod); len(controlledBy) > 0 {
w.Write(LEVEL_0, "Controller:\t%s\n", controlledBy)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Controller" is probably one of the most overused words in the community. How about "Controlled By"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

0xmichalis
0xmichalis reviewed Mar 10, 2017
View reviewed changes
pkg/printers/internalversion/describe.go Outdated
@@ -1338,6 +1351,9 @@ func describeReplicaSet(rs *extensions.ReplicaSet, events *api.EventList, runnin
w.Write(LEVEL_0, "Selector:\t%s\n", metav1.FormatLabelSelector(rs.Spec.Selector))
printLabelsMultiline(w, "Labels", rs.Labels)
printAnnotationsMultiline(w, "Annotations", rs.Annotations)
if controlledBy := printController(rs); len(controlledBy) > 0 {
w.Write(LEVEL_0, "Controller:\t%s\n", controlledBy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@enisoc
Copy link
Member

enisoc commented Mar 20, 2017

@janetkuo Since this is not release-blocking, we should move it to the v1.6.1 or v1.7 milestone.

@janetkuo janetkuo modified the milestones: v1.7, v1.6 Mar 20, 2017
@janetkuo janetkuo force-pushed the kubectl-describe-controllerRef branch from 57703ae to b29135e Compare April 11, 2017 22:42
@janetkuo
Copy link
Member Author

@Kargakis PTAL

0xmichalis
0xmichalis reviewed Apr 12, 2017
View reviewed changes
pkg/printers/internalversion/describe.go
return ""
}

func printCreator(annotation map[string]string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if created-by is used for anything else that the controller ref cannot. Can you spawn a separate issue to deprecate it and start removing it from places we can while keeping backwards compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #44407

@0xmichalis
Copy link
Contributor

/lgtm but I thought we wanted to deprecate created-by in favor of controller ref. Separate issue for discussing.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2017
@eparis
Copy link
Contributor

eparis commented Apr 12, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, janetkuo, kargakis

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit c9014b1 into kubernetes:master Apr 12, 2017
@mengqiy mengqiy mentioned this pull request Jun 20, 2017
Closed
This was referenced Nov 28, 2017
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmichalis 0xmichalis 0xmichalis left review comments

Assignees

@0xmichalis 0xmichalis

@enisoc enisoc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
8 participants
@janetkuo @k8s-reviewable @smarterclayton @enisoc @0xmichalis @eparis @k8s-github-robot @k8s-ci-robot