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

Use client, not server version for server dry run flag #793

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

timothysmith0609
Copy link
Contributor

@timothysmith0609 timothysmith0609 commented Jan 27, 2021

What are you trying to accomplish with this PR?
Perf testing a new 1.18 core cluster exposed a sleeping error inside the logic we use to determine the appropriate kubectl flag for specifying server-side dry-running. Prior to this PR, the server version is checked against 1.18+, when it should be the client version. Unfortunately this issue didn't appear in CI (since client/server are locked together there) and was missed on the 2.1.4 release.

How is this accomplished?
I verified the client/server error by attempting a deploy using various configurations of kubectl client and server versions
Screen Shot 2021-01-26 at 7 40 48 PM

As expected, the client version determines (somewhat obviously) the format of the flag we should pass in.

What could go wrong?
It already went wrong, unfortunately.

@timothysmith0609 timothysmith0609 requested a review from a team as a code owner January 27, 2021 00:41
Copy link
Contributor

@karanthukral karanthukral left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

test/unit/krane/ejson_secret_provisioner_test.rb Outdated Show resolved Hide resolved
@timothysmith0609 timothysmith0609 mentioned this pull request Jan 27, 2021
@timothysmith0609 timothysmith0609 merged commit 4935c35 into master Jan 27, 2021
@timothysmith0609 timothysmith0609 deleted the fix_kubectl_version_check_for_dry_run branch January 27, 2021 19:51
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.

None yet

3 participants