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

Introduce events flag for describers #24554

Conversation

ingvagabund
Copy link
Contributor

@ingvagabund ingvagabund commented Apr 20, 2016

Printing events for a given object is not always needed. Thus, introducing --show-events=false to kubectl describe to skip events printing.

Fixes: #24239

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 20, 2016
@@ -53,7 +53,13 @@ import (
// if the output could not be generated. Implementers typically
// abstract the retrieval of the named object from a remote server.
type Describer interface {
Describe(namespace, name string) (output string, err error)
Describe(namespace, name string, describerSetting *DescriberSetting) (output string, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for describerSetting to be a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, no. Later, DescriberSetting could contain non-pointer variables you would not want to copy.

Any reason for it not to be a pointer?

Copy link
Contributor

Choose a reason for hiding this comment

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

We avoid pointers in apis unless the absense of a value (nil) is something we care about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will update the PR.

@ingvagabund
Copy link
Contributor Author

ingvagabund commented Apr 21, 2016

There are three objects that has no unit-tests at all: HorizontalPodAutoscalerDescriber, IngressDescriber, JobDescriber. I could not found proper fake client for them. Could be worth writing one.

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch 2 times, most recently from 824d311 to 7120cd7 Compare April 21, 2016 13:23
@ncdc
Copy link
Member

ncdc commented Apr 21, 2016

cc @kubernetes/rh-cluster-infra @kubernetes/kubectl @kubernetes/rh-ux


// DescriberSetting holds display configuration for each object
// describer to control what is printed.
type DescriberSetting struct {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be pluralized - DescriberSettings. Or maybe DescriberOptions.

Copy link
Contributor Author

@ingvagabund ingvagabund Apr 21, 2016

Choose a reason for hiding this comment

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

I though setting is uncountable till this moment. Looks like google says "use plural if there are more things to set". DescriberConfig would work too. If the describer is meant just to "render", rendering settings is quite common. I like this comment [1]. DescriberPreferences is the closest to the behaviour from the point of the describer. On the other hand DescriberOptions makes more sense from the point of the kubectl describe command as events=true is by default and changing to false is providing option to change it.

DescribeOptions is already used so DescriberOptions counts out. Let's use DescriberSettings if noone against :).

[1] http://stackoverflow.com/questions/2074384/options-settings-properties-configuration-preferences-when-and-why/28812395#28812395

@ncdc
Copy link
Member

ncdc commented Apr 21, 2016

Need to run hack/update-generated-docs.sh.

Could you also please add some tests to hack/test-cmd.sh testing usage of this new flag, if possible?

@ingvagabund
Copy link
Contributor Author

ingvagabund commented Apr 21, 2016

@ncdc, yeah, I run that command at the end of our standup and blew my computer away. Sometimes the go compiler just eats all my memory. Was thinking about hack/test-cmd too. It is basically entire cluster set up without bare metal. A lot of objects created. Will put my eye on it tomorrow morning.

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch 2 times, most recently from 74f9146 to 5ac2705 Compare April 22, 2016 12:18
@ncdc
Copy link
Member

ncdc commented Apr 22, 2016

There are three objects that has no unit-tests at all: HorizontalPodAutoscalerDescriber, IngressDescriber, JobDescriber. I could not found proper fake client for them. Could be worth writing one

Create separate issues for each one?

@ingvagabund ingvagabund changed the title WIP: introduce events flag for describers Introduce events flag for describers Apr 25, 2016
@j3ffml
Copy link
Contributor

j3ffml commented Apr 25, 2016

cc @janetkuo

@derekwaynecarr
Copy link
Member

@@ -93,11 +94,12 @@ func NewCmdDescribe(f *cmdutil.Factory, out io.Writer) *cobra.Command {
kubectl.AddJsonFilenameFlag(cmd, &options.Filenames, usage)
cmdutil.AddRecursiveFlag(cmd, &options.Recursive)
cmd.Flags().StringP("selector", "l", "", "Selector (label query) to filter on")
cmd.Flags().BoolVar(&describerSettings.ShowEvents, "events", true, "If true, display also object events.")
Copy link
Member

@derekwaynecarr derekwaynecarr Apr 25, 2016

Choose a reason for hiding this comment

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

I prefer we call the flag show-events, we use that convention already with the show-all flag, and it parallels the name in the describer settings.

I would update the text to be If true, display events related to the described object.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch from dad88b7 to 712eeca Compare April 26, 2016 13:20
@ncdc
Copy link
Member

ncdc commented Apr 26, 2016

@k8s-bot test node e2e experimental issue: #23916

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch from 712eeca to dabee4d Compare April 26, 2016 15:58
},
}, events),
},
//"HorizontalPodAutoscalerDescriber": &HorizontalPodAutoscalerDescriber{
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to comment this out?

Copy link
Contributor Author

@ingvagabund ingvagabund Apr 26, 2016

Choose a reason for hiding this comment

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

I have changed all the three describers into TODO note. Opened separate issue for them [1].

[1] #24731

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch 2 times, most recently from 3a78128 to a595332 Compare April 27, 2016 08:13
@eparis
Copy link
Contributor

eparis commented Apr 29, 2016

/me has no idea what the bot did in #24554 (comment)

@eparis eparis removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 29, 2016
@j3ffml
Copy link
Contributor

j3ffml commented Apr 29, 2016

Looks good, but please squash commits.

@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch from be01d0b to a6a4b1d Compare April 29, 2016 17:24
@j3ffml j3ffml added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Apr 29, 2016
@ingvagabund
Copy link
Contributor Author

New PetSetDescriber introduced, not covered in this PR.

Introduce DescriberSettings for Describer display options
Introduce --show-events flag and DescriberSettings in Describer methods
Introduce unit-tests
Regenerated kubectl describe docs
Add events flag tests to test-cmd.sh

Signed-off-by: [email protected]
Signed-off-by: [email protected]
@ingvagabund ingvagabund force-pushed the dhodovska-events-flag-for-describers branch from a6a4b1d to dd2c9c5 Compare May 6, 2016 09:44
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. labels May 6, 2016
@ingvagabund
Copy link
Contributor Author

@ncdc @eparis @jlowdermilk can you reset lgtm flag?

@smarterclayton
Copy link
Contributor

LGTM

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2016
@k8s-github-robot k8s-github-robot assigned bgrant0607 and unassigned j3ffml May 6, 2016
@k8s-bot
Copy link

k8s-bot commented May 8, 2016

GCE e2e build/test passed for commit dd2c9c5.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 8, 2016

GCE e2e build/test passed for commit dd2c9c5.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.