GB UI - default settingsYaml to null #116
Conversation
…terfering with installations using settings.
| # to the chart and its templates, including the app version. | ||
| # Versions are expected to follow Semantic Versioning (https://semver.org/) | ||
| version: 0.5.7 | ||
| version: 0.5.8 |
There was a problem hiding this comment.
Have any changes been made to the gameboard-api chart? I'm not sure that this version needs to be bumped.
There was a problem hiding this comment.
I was told to increment all the charts for a single app at once, regardless if one half doesn't have chart changes - I'm happy to not do this if that's preferable. #helmczart?
There was a problem hiding this comment.
Pushed a commit to this PR to roll this change back.
| appname: Gameboard | ||
| ## NOTE: If specified here, will override the generated settings.json for installations that are using | ||
| ## the `settings` key. Change with caution. | ||
| settingsYaml: null |
There was a problem hiding this comment.
Can this be changed to settingsYaml: {} to match other empty dictionary values? It's also more descriptive when someone needs to override with actual values.
There was a problem hiding this comment.
This is where it's clear that I'm playing out of my depth, but the answer to this is "I'm not sure" 🤣 I discovered the hard way this morning that there are some layers here I wasn't familiar with.
The config map for this settings file is here. It checks whether settingsYaml is defined and uses that if present, but falls back to settings if not. Since an empty dictionary is a truthy value (according to my brief research), this check would pass, and the value of settingsYaml will be used over anything in settings. I saw this in vivo today, where we changed an existing installation's settingsYaml to {}, but that empty value was still being used over a fully defined settings key.
I think there's something there I'm still not getting, because before I started monkeying with this, this key was set to {} in this repo's values file. I'm very willing to change this back to {}, but do we know if that would cause problems for installations which don't define a settingsYaml value at all and instead rely on settings?
There was a problem hiding this comment.
Update: It looks like I misunderstood - in a Helm template, {} is a falsey value, so that should work. I'll reach out offline, but I've updated the PR based on this feedback.
charts/gameboard/values.yaml
Outdated
| appname: Gameboard | ||
| ## NOTE: If specified here, will override the generated settings.json for installations that are using | ||
| ## the `settings` key. Change with caution. | ||
| settingsYaml: null |
There was a problem hiding this comment.
Same comment as the UI settingsYaml value.
|
I think we need to bump the appVersion of both charts to 3.35.0. I assume that is the version of GB that we are targeting for the oidc changes. If so, then we should also bump the chart version for the api chart. |
|
Version 3.35.0 was released after this PR. I plan to PR new chart/app versions for that separately, but I wanted to resolve this question first, as it's a separate issue. Would you rather I add the changes for GB 3.35.0 and TM 2.5.0 to this PR? |
This rolls back a change that supplied some defaults to
settingsYamlfor GBUI. The issue is that if this value is specified, any downstream apps that are usingsettingsto configure their apps will have them overridden by the defaultsettingsYamlfrom our values.Changed this value to
nullin the greater GB chart and the GB UI chart, and added a note explaining this until we find a better way forward for settings more generally.