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

Add Monitoring v0.1.4 #357

Merged
merged 4 commits into from
Oct 14, 2020
Merged

Add Monitoring v0.1.4 #357

merged 4 commits into from
Oct 14, 2020

Conversation

aiyengar2
Copy link
Contributor

@aiyengar2 aiyengar2 commented Oct 12, 2020

mrajashree
mrajashree previously approved these changes Oct 12, 2020
@aiyengar2 aiyengar2 requested review from dramich and removed request for dramich October 13, 2020 02:51
@aiyengar2
Copy link
Contributor Author

aiyengar2 commented Oct 13, 2020

@deniseschannon since v0.1.3 is already in release/v2.4, does this PR need to have a base copy -> v0.1.4?

Not sure if we need a new chart for this since

  1. it's a small change
  2. it's just introducing a previously unsettable field (see Allow users to overwrite operator-init values rancher#29478) that would not have impacted anyone who is currently using monitoring (since, if they were impacted, they wouldn't have been able to successfully deploy monitoring in the first place)
  3. anyone who needs this feature such as the person who opened up this ticket would anyways be installing / reinstalling monitoring to use it.

If we want to be safe, it should be easy to make the change though.

@aiyengar2
Copy link
Contributor Author

aiyengar2 commented Oct 13, 2020

Also, the changes to this chart will have no effect unless rancher/rancher#29478 is merged since any values supplied to operator-init (like this newly exposed value) would be ignored by operatorHandler (i.e. the Rancher controller that actually handles the logic for deploying the CRDs + the Prometheus Operator pod) either way till that change goes in.

@dramich
Copy link
Contributor

dramich commented Oct 13, 2020

@aiyengar2 Yes, it needs to be a new version, we are making changes to something that is already released.

@aiyengar2
Copy link
Contributor Author

@aiyengar2 Yes, it needs to be a new version, we are making changes to something that is already released.

Updated!

@aiyengar2 aiyengar2 changed the title Add toleration support to operator-init Add Monitoring v0.1.4 Oct 13, 2020
@aiyengar2
Copy link
Contributor Author

@aiyengar2 Yes, it needs to be a new version, we are making changes to something that is already released.

Since we're releasing a new version with this PR, I slightly modified the purpose of this PR to address both rancher/rancher#27253 and rancher/rancher#29290 since they are both minor changes.

Merging this PR should be blocked based on resolving https://github.com/rancherlabs/eio/issues/280 and ensuring that the new prometheus-auth image exists on DockerHub.

But no code changes will be introduced after that so this PR is still ready for review.

Copy link
Contributor

@pennyscissors pennyscissors left a comment

Choose a reason for hiding this comment

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

lgtm

@aiyengar2
Copy link
Contributor Author

Merging this PR should be blocked based on resolving rancherlabs/eio#280 and ensuring that the new prometheus-auth image exists on DockerHub.

https://hub.docker.com/layers/rancher/prometheus-auth/v0.2.1/images/sha256-b8d21dad8923689177c526ab0291d30e22180a754b0b3ca8eb67dd142063c87a?context=explore exists

@aiyengar2 aiyengar2 merged commit ed845a4 into rancher:dev-v2.4 Oct 14, 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
4 participants