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

Adding dependency checker for kubectx and kubens #92

Merged
merged 2 commits into from Oct 22, 2018

Conversation

@sarcasticadmin
Copy link
Contributor

commented Oct 19, 2018

Overview

To address #5 Misunderstood the issue here

Rather than fail after attempting to get a context or namespace Im proposing to fail when kubectl is not found in the PATH.

Let me know what you think!

Testing

$ brew unlink kubectl
$ ./kubectx; echo $?
kubectl not installed
1
$ ./kubens; echo $? 
kubectl not installed
1

Also submitted the CLA as described in CONTRIBUTING.md

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Oct 19, 2018

Hello, thanks for your PR.

How are you installing kubectx (on which platform/package manager)? For macOS, our Homebrew package has a dependency on kubectl, so that'll be installed automatically.

In other platforms (like Linux) I'm not sure how realistic is it to expect that people will have kubectx; but not kubectl. 😃

To address #5

This PR isn't addressing that issue. That issue is happening because bash is not using set -e when you call a function in a for-loop, like for c in $(get_contexts); do ....

As far as your PR is concerned, you can simply do this (and I prefer this):

if ! hash kubectl 2>/dev/null; then
   echo >&2 "kubectl is not installed'
   exit 1
fi
@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

@ahmetb Im just clone the repo and running the scripts from the repository. Considering this is a bash script I think its likely other users will use this script outside of a package manager as well regardless of OS.

My fault, I miss understood the requirements for #5.

Thank for the feedback and Ill fixup the PR with your suggestions. 👍

@sarcasticadmin sarcasticadmin force-pushed the sarcasticadmin:sa/dep-check branch from 14e0df1 to 083e56f Oct 20, 2018

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

@ahmetb let me know what you think!

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Oct 20, 2018

@sarcasticadmin The code looks good but I'm still not sure if anyone is actually trying to use kubectx without kubectl. :)

The issue #5 as I said is because we lose the exit code of the function while we call it from a for-loop in bash script, so set -e doesn't properly cause script to exit and keeps going on. That's the actual issue.

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

@ahmetb sorry I was under the impression that you were still interested in adding a check for the existence of kubectl. I understand that its pretty obvious that there isnt much use for kubectx without kubectl. But Ive found its common for these kinds of tools to check for the required binaries (failing gracefully if missing) since there are so many different ways one can configure their environment. For example helmsman has a similar check: https://github.com/Praqma/helmsman/blob/master/init.go#L108-L114

If its still something you think isnt necessary you can close this one. I'll follow up with a fix for #5 in a different PR 👍

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2018

No I think this PR is good. It just isn't a fix for #5. :)

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2018

Manually verified CLA (not sure why CLA bot didn't kick in).

@ahmetb ahmetb added the cla: yes label Oct 22, 2018

@ahmetb ahmetb merged commit 517dae9 into ahmetb:master Oct 22, 2018

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