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 ingress-nginx plugin #133

Merged
merged 2 commits into from Apr 9, 2019

Conversation

Projects
None yet
3 participants
@alexkursell
Copy link
Contributor

commented Apr 8, 2019


This PR add the kubectl plugin for https://github.com/kubernetes/ingress-nginx.

Checklist for plugin developers:

  • Read the Plugin Naming Guide (for new plugins)
  • Verify the installation from URL or a local archive works (kubectl krew install --manifest=[...] --archive=[...])
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@alexkursell This is excellent and makes me super happy that Krew can host a first-party kubectl plugin from another Kubernetes sub-project.

Do you mind attending to the SIG CLI meeting and giving a short 5-10 min talk about your experience with Krew? Maybe I should ask for this after you release a few more versions and experience some friction along the way.

metadata:
name: ingress-nginx
spec:
shortDescription: Interact with ingress-nginx

This comment has been minimized.

Copy link
@ahmetb

ahmetb Apr 8, 2019

Contributor

We recently added a homepage field, too.
Do you mind pointing it to the plugin documentation?

This comment has been minimized.

Copy link
@alexkursell

alexkursell Apr 8, 2019

Author Contributor

Once kubernetes/ingress-nginx#3982 is merged, I'll add a link to the docs.

This comment has been minimized.

Copy link
@alexkursell

alexkursell Apr 9, 2019

Author Contributor

fixed

files:
- from: "*"
to: "."
bin: "./kubectl-ingress_nginx"

This comment has been minimized.

Copy link
@ahmetb

ahmetb Apr 8, 2019

Contributor

I haven't tested this but it's a bit odd that the windows release is not distributed as .exe inside the tarball.

Krew will link it as .exe correctly, but I'm not sure if the symlink will work correctly because the target file isn't going to be named as .exe (see kubernetes-sigs/krew#131).

Basically, we don't know what happens on windows if we make a symlink to a source file named foo as bar.exe, and invoke bar.exe. Does foo actually get executed without needing an .exe extension?

This comment has been minimized.

Copy link
@alexkursell

alexkursell Apr 8, 2019

Author Contributor

I just booted into windows to try it, and it seems like it won't even download it properly:

 C:\Users\alex\.krew\bin\kubectl-krew.exe install --manifest ingress-nginx.yaml
Installing plugin: ingress-nginx
W0408 19:40:53.067709    4712 install.go:132] failed to install plugin "ingress-nginx": failed to dowload and move during installation: failed to move files: failed moving files: could not rename file from "C:\\Users\\alex\\AppData\\Local\\Temp\\krew-downloads\\ingress-nginx\\kubectl-ingress_nginx" to "C:\\Users\\alex\\AppData\\Local\\Temp\\krew-temp-move626560575\\kubectl-ingress_nginx": rename C:\Users\alex\AppData\Local\Temp\krew-downloads\ingress-nginx\kubectl-ingress_nginx C:\Users\alex\AppData\Local\Temp\krew-temp-move626560575\kubectl-ingress_nginx: The process cannot access the file because it is being used by another process.
F0408 19:40:53.067709    4712 root.go:52] failed to install some plugins: [ingress-nginx]

I installed sniff just fine, so I'm assuming it's the lack of a .exe extension. I might just leave Windows out at this point in time and fix this for the next release.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Apr 9, 2019

Contributor

That sounds like a krew bug. We are usually careful around Windows file opening semantics, but looks like there's a problem.
Do you mind filing a bug to https://github.com/kubernetes-sigs/krew/issues/new with -v=10 flag? I wonder if other plugin installation are also failing.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Apr 9, 2019

Contributor

Just saw the second part of your comment.
Can you try creating a .zip file with the windows binary (named *.exe) and run install again with --archive foo.zip? If this works, I'd be surprised, it doesn't explain the "file is used by another process" error.

This comment has been minimized.

Copy link
@alexkursell

alexkursell Apr 9, 2019

Author Contributor

I'll give this a try when I get home tonight. For now, I've just removed windows support entirely.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Apr 9, 2019

Contributor

OK, let me know if you want me to go ahead and merge this.

@alexkursell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Do you mind attending to the SIG CLI meeting and giving a short 5-10 min talk about your experience with Krew? Maybe I should ask for this after you release a few more versions and experience some friction along the way.

I'd love to! I do think it makes sense to wait a few releases though. My usage of krew up until this point has been pretty minimal so far.

@alexkursell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@ahmetb I think this is good to merge unless you have any other concerns.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Apr 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, alexkursell

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 0d5b33a into kubernetes-sigs:master Apr 9, 2019

2 of 3 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation alexkursell authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexkursell alexkursell deleted the alexkursell:nginx-0.24.0 branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.