-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
e346352
to
b401daa
Compare
CI is failing, and this appears to be the error:
I wonder if that's caused by #18, which renamed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -105,11 +105,6 @@ spec: | |||
targetPort: 5601 | |||
interval: 30s | |||
prometheusSpec: | |||
image: | |||
tag: v2.9.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what prevented your change in mesosphere/charts#309 from taking effect I'm guessing?
But rather than delete entirely, should we still pin to a much more recent stable image? Or is that already covered since we're pointing to the chart 8.3.3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was causing prometheus-operator to deploy an older version of Prometheus that couldn't handle some of the updates to the chart.
The chart's default images have been updated as part of the chart upgrade, so using defaults means we're getting the new versions. I decided to remove the tag override rather than update it, since AFAIK we no longer need to pin a specific version, and using the defaults should be easier to maintain.
Oh good catch. I wonder if we need to keep the addon for the older Prometheus version, since it won't end up in a GA release. I'll just rename the dir to |
Sounds good. Hopefully no issues 🤞 I wonder if in the future we'll want to provide multiple versions in case users want to use an older chart (for eg bc they can't upgrade to the latest one yet). mesosphere/universe already has a pretty good system for this. |
please rebase, should help with the test. |
This upgrades Prometheus past the chart's default of 2.13.1. 2.14.0 brings some UI enhancements that we need.
This is necessary for the Thanos sidecar to be deployed.
ac32c96
to
dbce7a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once CI is happy, ✔️
As a non-blocker, I did want to point out that this is probably one of the last times before all the lights are on with this repo that we'll be able to make significant updates to an addon like this in place. In the future it will be expected that we make a new revision as per the documentation but that's waiting on the first end-user of this repo (konvoy) to fully integrate this repo. Just context, no action needed.
There's a remaining issue with mesosphere-backup/kubeaddons-configs#368, so I'll put the WIP label back on this PR until that's ironed out. |
The plan is to merge mesosphere-backup/kubeaddons-configs#368 as it is and then follow up with another PR to fix the bug it introduces. |
This corresponds to the kubeaddons-configs PR at mesosphere-backup/kubeaddons-configs#368.
Testing
See the testing section in the description for mesosphere/charts#309.