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

Fix deprecated api which is incompatoble with Kubernetes > 1.16 #75

Merged
merged 1 commit into from Jan 2, 2020

Conversation

Flowkap
Copy link
Contributor

@Flowkap Flowkap commented Dec 5, 2019

Hey, we wanna use the cahrt in Kubernetes 1.16 and your deployment still uses the deprecated api version which is to finally be removed.

https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/

Greetz Flowkap :)

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #75   +/-   ##
=======================================
  Coverage   70.16%   70.16%           
=======================================
  Files           2        2           
  Lines         181      181           
=======================================
  Hits          127      127           
  Misses         48       48           
  Partials        6        6

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 94f7648...bde5096. Read the comment docs.

@@ -2,4 +2,4 @@ apiVersion: v1
appVersion: "1.1.4"
description: A Helm chart for Kubernetes
name: prometheus-msteams
version: 0.3.0
version: 0.3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Flowkap thank you for this contribution. I am wondering whether this is a patch version API change but rather a major version API change? I assume apiVersion: apps/v1 is not compatible with Kubernetes versions <1.16, is it?

Copy link
Contributor Author

@Flowkap Flowkap Dec 8, 2019

Choose a reason for hiding this comment

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

It is I believe since 1.9 (checked in my link). Don't think Kubernetes <= 1.8 is still relevant. The operator would not even run. Therefore and as the helm chart doesn't change it's own config API I chose lesser version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I should have clicked your link first ;). I think this looks good then. @bzon please provide a lgtm, then we can merge.

@Knappek Knappek requested a review from bzon December 8, 2019 18:46
Copy link
Collaborator

@bzon bzon left a comment

Choose a reason for hiding this comment

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

Please see the recommended change. 👍

@Flowkap
Copy link
Contributor Author

Flowkap commented Dec 12, 2019

Kubernetes Operator is not even supporting any Kubernetes version not supporting current API. Why keep the deprecated one at all. I mean sure could be configurable but in this case has very limited use.

@bzon
Copy link
Collaborator

bzon commented Dec 12, 2019

Kubernetes Operator is not even supporting any Kubernetes version not supporting current API. Why keep the deprecated one at all. I mean sure could be configurable but in this case has very limited use.

What Kubernetes Operator are we talking about here? I'm just concerned that this upgrade may break existing installations. Though it is very unlikely that people are still using old versions < v1.9, let's just use the new one apps/v1 as the default and keep it configurable just to be on the safe end. We can hard lock and remove it in the future. 😄

@Flowkap
Copy link
Contributor Author

Flowkap commented Dec 12, 2019

I mean Prometheus Operator.

It's your project. Still I find this unnecessary. Existing installations so not need to upgrade helm. We can't use the last 2 Kubernetes versions. Making this configurable is bloated imho. At least I won't change my PR. Feel free to reject. We're using my fork atm working fine.

Don't wanna be harsh but I really disagree in keeping an old outdated API configurable.

If you play the game of Kubernetes you can't keep totally outdated versions live. 1.8 is not even secure at all with the known issues.

@bzon
Copy link
Collaborator

bzon commented Dec 12, 2019

I mean Prometheus Operator.

It's your project. Still I find this unnecessary. Existing installations so not need to upgrade helm. We can't use the last 2 Kubernetes versions. Making this configurable is bloated imho. At least I won't change my PR. Feel free to reject. We're using my fork atm working fine.

Don't wanna be harsh but I really disagree in keeping an old outdated API configurable.

If you play the game of Kubernetes you can't keep totally outdated versions live. 1.8 is not even secure at all with the known issues.

I find it interesting what makes you disagree so much with a configurable default value using the new one. You are missing the point of my recommended change. It's not even a big deal and you don't even have to bring stuff like that. 😓

As a maintainer of an opensource project, you should find the balance between the complexity of a change and the possible causes of breaking some environments. This is a community.

@Flowkap
Copy link
Contributor Author

Flowkap commented Dec 12, 2019

If anything this is going to break outdated installations. Making everything configurable is bloating a lot. Especially for new people. In this case I just disagree. From all 13 open source charts we use none had API configurable as far as I can tell. And none wasn't Kubernetes 1.16 ready.

Sorry 🤷🏻‍♂️

@Knappek
Copy link
Collaborator

Knappek commented Jan 2, 2020

thanks again @Flowkap for this PR. I'll merge it and will parameterize the apiVersion in a separate PR similar what has been done in the nginx-ingress chart which seems to be quite common to implement.

@Knappek Knappek merged commit 8ff2967 into prometheus-msteams:master Jan 2, 2020
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