-
Notifications
You must be signed in to change notification settings - Fork 893
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
Optimize karmadactl get to output more information #1270
Conversation
Looks pretty good! Please cc me once ready for view. |
a22e781
to
2ae3ea8
Compare
Thanks, as I thought. I want to talk about using |
/cc @RainbowMango |
pkg/karmadactl/get.go
Outdated
@@ -188,6 +200,7 @@ type OtherPrint struct { | |||
} | |||
|
|||
// Run performs the get operation. | |||
//nolint:gocyclo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan to remove this nolint:gocyclo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will do it in this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this nolint:gocyclo
need to split code that make code unreadable and complicated, so not removed for now and continue to find a good way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with it. Could you please file an issue for this? You know, the technical debt is pretty easy to forget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, add a issue that list all nolint:gocyclo
in codes as items and plan to remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not list all, just focus on the new one would be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the example also needs to be updated?
./karmadactl get --help
Display one or many resources
Usage:
karmadactl get [NAME | -l label | -n namespace] [flags]
Examples:
# List all pods in ps output format
karmadactl get pods
# List a single replicasets controller with specified NAME in ps output format
karmadactl get replicasets nginx
# List deployments in JSON output format, in the "v1" version of the "apps" API group
karmadactl get deployments.v1.apps
# List all replication controllers and services together in ps output format
karmadactl get rs,services
# List one or more resources by their type and names
karmadactl get rs/nginx-cb87b6d88 service/kubernetes
I'm ok if you are going to do it by following PR. Just a question or reminder.
Right, I add it right away. |
88d98fe
to
779a8eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lonelyCZ
This branch has conflicts that must be resolved.
Ok, I will fix it. |
ea46568
to
13e545c
Compare
Signed-off-by: lonelyCZ <[email protected]>
13e545c
to
684d6c9
Compare
/cc @RainbowMango The conflict has been fixed and the |
Works as expected. |
I updated the PR description to fixes the #1219. The following work(should be tracked by another issue) is not in the scope of the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
Ok, thanks! :) |
Signed-off-by: lonelyCZ [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
Optimize output content.
Which issue(s) this PR fixes:
Fixes #1219
Special notes for your reviewer:
Step1: Optimize output.
Step2: replace
secret
withproxy
to access member clusters(include pull mode cluster).Does this PR introduce a user-facing change?: