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 describe control to all k8s resources #3589

Merged
merged 3 commits into from
May 17, 2019

Conversation

qiell
Copy link
Contributor

@qiell qiell commented Apr 1, 2019

This PR will add a new control i.e. Describe
which will describe a k8s resource similar to kubectl describe.
fixes: #1143

Signed-off-by: Akash Srivastava [email protected]

qiell added 2 commits March 31, 2019 15:46
Commands used:-
gvt fetch --no-recurse -tag v1.13.0 k8s.io/kubernetes/pkg/kubectl/describe
gvt fetch --no-recurse -tag v1.13.0 k8s.io/kubernetes/pkg/kubectl/util
gvt fetch --no-recurse -tag v1.13.0 k8s.io/kubernetes/pkg/kubectl/scheme
gvt fetch --no-recurse -tag kubernetes-1.13.0 k8s.io/cli-runtime/pkg/genericclioptions
gvt fetch --no-recurse -tag v1.0.0 github.com/fatih/camelcase

Signed-off-by: Akash Srivastava <[email protected]>
This commit will add a new control i.e. Describe
which will describe a k8s resource similar to kubectl describe.

Signed-off-by: Akash Srivastava <[email protected]>
@fbarl fbarl self-requested a review April 1, 2019 11:41
@qiell
Copy link
Contributor Author

qiell commented Apr 1, 2019

Screenshot from 2019-04-01 19-01-03

Copy link
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

How did we aquire a dependency on vendor/k8s.io/cli-runtime/pkg/genericclioptions ?
We aren't actually running the kubectl CLI.

case "<pod>":
f = r.CapturePod(r.describePod)
case "<service>":
f = r.CaptureService(r.describeService)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be a lot of repetition going on here - could it be done in a data-driven way?
E.g. a table containing function pointers like ParseServiceNodeID, WalkServices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @bboreham
Will update the PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did we aquire a dependency on vendor/k8s.io/cli-runtime/pkg/genericclioptions ?

k8s.io/kubernetes/pkg/kubectl/describe/interface.go requires the genericclioptions package. Because of this we acquired this dependency.

Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

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

The UX looks good to me for now 👍

I'd also be happy if you addressed the comments from @bboreham before merging though :)

probe/kubernetes/reporter.go Show resolved Hide resolved
@fbarl
Copy link
Contributor

fbarl commented May 10, 2019

@qiell Did you encounter any blockers trying to address review comments? I think the PR is very close to being ready to merge, I'd be happy to help wrap things up if needed :)

@qiell
Copy link
Contributor Author

qiell commented May 10, 2019

Hi, @fbarl Thanks for your concern. I had one issue but @bboreham helped me to tackle that. I couldn't find time to catch up on this. Will try to put that change ASAP. If I face any blockers or issue, I'll let you know.

Thank you :)

Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

The only thing left to address is a comment from @bboreham (https://github.com/weaveworks/scope/pull/3589/files#r273417103), but let's create a separate issue to address that once this PR is merged.

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 this pull request may close these issues.

equivalent of kubectl describe
3 participants