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

[REVIEW] Proposal to fix subshell error handling #95

Merged
merged 2 commits into from Nov 7, 2018

Conversation

@sarcasticadmin
Copy link
Contributor

commented Oct 30, 2018

Overview

Proposal to address #5

My proposal is to check the return code of the problematic functions. If any of them return a non-zero, call exit_err immediately, print an error message to stderror and exit 1.

Testing

Validate that kubectx and kubens still function. I wasnt able to force kubectx and/or kubens to fail in the middle of gathering contexts or namespaces so I used this test function to test my concepts:

#!/usr/bin/env bash
set -o pipefail

exit_err() {    
   echo >&2 "${1}"        
   exit 1                    
} 

get_contexts() {
  echo -e "hello\nyay\nworld"
}

# Supposed to fail
get_contexts_fail() {
  echo -e "hello\nyay\nworld"
  tar -clv | sort -n
}

# Will Succeed 
cons=$(get_contexts) || exit_err "error on test 1"
for con in $cons; do
  echo $con
done

echo "finished get context"

# Will Fail
cons=$(get_contexts_fail) || exit_err "error on test 2"
for con in $cons; do
  echo $con
done

Running it to simulate how an error would return from a function like get_contexts in kubectx:

~$ bash test.sh
hello
yay
world
finished get context
tar: Cowardly refusing to create an empty archive
Try 'tar --help' or 'tar --usage' for more information.
error on test 2

@ahmetb if youve got any ideas for ways to simulate a failure during get_context or similar functions calling kubectl let me know. Hopefully this is more of what you're looking for to solve for the subshell issue.

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Oct 30, 2018

Thank you so much, I'll review and test this next week once I'm back from vacation.

An easier way to fail is to write a script called kubectl and put it in your path, and only fail when args are a specific value. Another good way to test is through kubens, just mess your kubeconfig file or turn off your internet.

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@ahmetb sounds great, Ill try out those testing tips myself and report back!

Enjoy your vacation!

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Nov 5, 2018

@sarcasticadmin I think overall this PR looks good. Have you had a chance to simulate the failure modes?

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@ahmetb awesome! I havent had a chance to test yet, Ill make some time this afternoon and get back to you with the results

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@ahmetb was able to test this tonight and things are working as expected!

I created a kubectl script that would randomly error when called to simulate failures in various parts of the script and put it into my PATH:

$ cat kubectl 
#!/usr/bin/env bash

exit $((RANDOM%2))

Then running kubectx I got the desired behavior for the failures:

$ ./kubectx; echo $?
error getting current context
1
$ ./kubectx; echo $?
error getting context list
1
$ ./kubectx; echo $?
error getting current context
1

Moving onto kubens I simulated the two scenarios you suggested:

Misconfiguring my kubeconfig and got the expected behavior:

$ ./kubens
Error from server (Forbidden): namespaces is forbidden: User "email@gmail.com" cannot list
namespaces at the cluster scope: Required "container.namespaces.list" permission.
error getting namespace list

Then without an internet connection:

$ ./kubens
Unable to connect to the server: dial tcp 111.111.1111.111:443: connect: network is unreachable
error getting namespace list

If theres anything else you'd like me to test just let me know

@ahmetb

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2018

@sarcasticadmin Thank you very much, this looks good.

If you have any interest in being a maintainer in this repo, I'd be happy to have you participate to the discussions. Click "Watch" on the repo to stay in the loop.

@ahmetb ahmetb merged commit 3aeb4e7 into ahmetb:master Nov 7, 2018

@sarcasticadmin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@ahmetb thanks for merging!

I'm definitely interesting in being a maintainer and helping out with the project! Looking forward to participating in future discussions 🍿

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.