Skip to content

Conversation

@peter-funktionIT
Copy link

More feature complete even when unable to reach server

More feature complete even when unable to reach server
Stupid error
# Echo as comment to make sure "kubectl completion" etc works in profile
echo -e "#WARNING: Wrapper unable to verify server version, using default version $DEFAULT_VERSION.\n"
$DEFAULT_CLIENT "$@"
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary else statement. use exit $?


if [ "$1" == "config" ]; then
$DEFAULT_CLIENT "$@"
exit $?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep config with default client to make sure we can always configure our kubectl and dont have to wait 30 seconds for an i/o timeout if cluster not reachable for example.

$DEFAULT_CLIENT "$@"
exit $?
VERSION_OUTPUT=$($DEFAULT_CLIENT version -o json 2>&1)
if grep -q "Unable to connect to the server" <<<"$VERSION_OUTPUT"; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on error message strings is fragile. Better to check the exit code != 0

@jonaz
Copy link
Member

jonaz commented Nov 28, 2018

On more thought i think using default version is bad. If we cannot talk to cluster fetching version we will not be able to talk to cluster got what other command we where using. So simple fix is just exit with error message if SERVER_VERSION is empty.

@peter-funktionIT
Copy link
Author

peter-funktionIT commented Nov 28, 2018 via email

@jonaz
Copy link
Member

jonaz commented Nov 28, 2018

Then its better to "whitelist" more stuff. For example if --help is in any argument always use default client. if first argument it help always use default client.

We only want to check server version for commands that talk to the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants