-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Prepare helm chart for 0.3.0 #73
Conversation
Can the chart be deployed in its current state? |
so, this is required? |
We should release 0.0.3 and use that image for the chart. Once #65 is merged, we're good to go. |
I've bumped the charts appVersion to v0.3.0 to prepare for release |
I've fixed a bug where the yaml couldn't be parsed if the service monitor is enabled but no labels were set. Now values like this are vaild: # -- Configure a service monitor for prometheus-operator
serviceMonitor:
# -- Enable the serviceMonitor
enabled: true
# -- Sets the scrape interval
interval: 30s
# -- Sets the scrape timeout
scrapeTimeout: 5s
# -- Additional label added to the service Monitor
labels: {} |
please look at #75, as that pr fixes e2e and allows us to properly test 0.3.0 internally |
* feat: simplify helm chart * fix: remove -n * fix: register healthcheck for e2e test * feat: separate checks and sparrow config * fix: e2e tests * fix: bug where env variables would not be read correctly * feat: allow mounting extra secrets as env variables * docs: envfromSecrets
Pls look at last 2 comments #75 |
there is still a bug with marshalling env variables correctly, it is semi fixed in viper 1.18.0 but was hidden behind a feature flag in 1.18.2 due to causing a different bug. That bug only appears when using fun stuff like pointer pointers, so we should be fine I think. |
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.
Shouldn't the chart work out-of-the-box without adjusting any values?
Currently that wouldn't be the case since the name and loader are mandatory:
$ helm upgrade sparrow chart/ -i --atomic --set image.tag=commit-b1dd2fc
Using config file: /config/.sparrow.yaml
{"time":"2024-01-18T19:08:59.750947469Z","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*Config).Validate","file":"/home/runner/work/sparrow/sparrow/pkg/config/validate.go","line":40},"msg":"The name of the sparrow must be DNS compliant"}
{"time":"2024-01-18T19:08:59.751057558Z","level":"ERROR","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*Config).Validate","file":"/home/runner/work/sparrow/sparrow/pkg/config/validate.go","line":47},"msg":"The loader http url is not a valid url"}
Error: error while validating the config: validation of configuration failed
...
error while validating the config: validation of configuration failed
Stream closed EOF for default/sparrow-75d554645d-ms8zq (sparrow)
I think we should still include required default configs to improve the user experience with this chart.
added some defaults, that do nothing |
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.
LGTM I'll do another PR for docs asap so we can release v0.3.0 today
Motivation
Since #69 added servicemonitors to the helm chart, we should also bump the chart version.
Changes
For additional information look at the commits.