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

Improve kubectl plugin #3870

Merged
merged 1 commit into from
Mar 14, 2019
Merged

Conversation

alexkursell
Copy link
Contributor

@alexkursell alexkursell commented Mar 7, 2019

Based on feedback in #3831, I've made the following additions to the plugin

  • Add --service and --deployment flags where appropriate for situations in which the ingress-nginx service or deployment names are not the defaults.
  • Add logs command to display the logs of an ingress-nginx controller
  • Add exec command to execute a command inside the controller
  • Add ssh command to get a shell inside the controller.
  • Add the ENDPOINTS column to ingresses to show the number of endpoints for the backend service (or N/A if it's an ExternalName). This count currently ignores some of the subtleties of endpoints where not every endpoint in a service is actually serving the particular port the ingress is using, but it's good enough for a rough count.

To get this merged, I feel like there are a few things left to consider:

  • Properly implementing exec and ssh (and logs too, with all of the flags it should support) might require duplicating uncomfortably large parts of kubectl functionality. It might be better to have those commands "fall-through" and just call kubectl directly behind the scenes with the correct pod name. I don't think this plugin will ever be used in a context where kubectl doesn't exist, but this warrants some discussion. edit exec, ssh, and logs have been replaced with kubectl fall-through
  • Add all of the supported kubectl logs flags to logs
  • Refactoring. main.go in particular is getting uncomfortably large and should probably be broken up into multiple files

Some things that should probably get done before the 0.24.0 release, but maybe not necessarily in this PR:

  • Adding e2e (and possibly unit) tests
  • Writing documentation

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 7, 2019
@aledbf
Copy link
Member

aledbf commented Mar 7, 2019

Properly implementing exec and ssh (and logs too, with all of the flags it should support) might require duplicating uncomfortably large parts of kubectl functionality.

Yes, please use kubectl and remove the client-go code. This not only removes code from this repo but is consistent with the kubectl binary the user is already using.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 8, 2019
@alexkursell alexkursell changed the title [WIP] Improve kubectl plugin Improve kubectl plugin Mar 11, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2019
@alexkursell
Copy link
Contributor Author

alexkursell commented Mar 11, 2019

@aledbf, @ElvinEfendi I think this PR is about ready feature-wise. Let me know if you have any concerns or any more features you want me to add.

@aledbf
Copy link
Member

aledbf commented Mar 14, 2019

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019
@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, alexkursell, ElvinEfendi

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

@k8s-ci-robot k8s-ci-robot merged commit 2dbc1ea into kubernetes:master Mar 14, 2019
@alexkursell alexkursell deleted the improve-plugin branch March 14, 2019 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants