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

Updating kubernetes api version to support the latest apps/v1 version #65

Merged
merged 2 commits into from
Dec 12, 2019

Conversation

r1sharma
Copy link
Contributor

This PR is fix related to the issue #63 & #62 . A part of these issues have been already addressed in Pull request : #53

Changes include:

  • Updated chaosk8s\actions.py & chaosk8s\probes.py
  • Updated related tests too.

@codecov-io
Copy link

codecov-io commented Dec 11, 2019

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   77.23%   77.23%           
=======================================
  Files           4        4           
  Lines         325      325           
=======================================
  Hits          251      251           
  Misses         74       74

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5de1aa...b80d95a. Read the comment docs.

Signed-off-by: rahulsharma <rahulsharmaoist@gmail.com>
Copy link
Contributor

@Lawouach Lawouach left a comment

Choose a reason for hiding this comment

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

Good call.

A few questions:

  • could you please add a changelog entry to explain why this change is needed please?
  • could you try to explain what may happen for older clusters? what's the minimal version of the Kubernetes API does this work against? I've seen the go code making a call first to the new endpoint and, when it fails, falling back to the old API. should we follow the same path?

I'm surprised tests weren't impacted. maybe we are missing some...

@r1sharma
Copy link
Contributor Author

r1sharma commented Dec 12, 2019

@Lawouach Sure will update this PR with changelog later today.
Please take a look at the notes here from Kubernetes on deprecated apis removed from v1.16 and future version of kubectl. I am using version 1.16+ for kubectl and chaos toolkit keeps failing for that even after your fix here as current code points to apps/v1beta1.

@Lawouach
Copy link
Contributor

Makes sense and your fix seems the right one. Wondering about backward compatibility.

@r1sharma
Copy link
Contributor Author

previous version of chaosk8s will be compatible. right?

@Lawouach
Copy link
Contributor

True although this isn't my philosophy here. But, reading your link again, they mention the apps/v1 version has been around since K8S 1.9 which is "old" (with Kubernetes, 18 months is ancient times).

I might work on a change, after yours, that will try to support both by looking up the version of the server first.

@r1sharma
Copy link
Contributor Author

Sounds good

@Lawouach Lawouach merged commit 79ceccf into chaostoolkit:master Dec 12, 2019
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