Facades: Allow empty charm config values #13130
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The following changes allow the charm config as a map and the charm config as a yaml to work as intended. The code previously assumed that there was always a yaml and that the charm config map would always overwrite it. The problem occurs when there isn't a charm yaml and it would blindly overwrite values of the yaml. This caused empty strings to be converted to nil, which then caused the --reset flow to be triggered. The --reset flow essentially says, use the config defaults when you see nil, by deleting any user config. That's something that we didn't want to trigger. The change causes us to exit early if there isn't a yaml. There is a much deeper problem and one that we will need to understand. API versioning should essentially not touch the old code OR at the very least have enough testing to ensure that any new changes keeps the old behaviour. 100% code coverage in these scenarios would be beneficial to ensure that we didn't trip up here. The problem with attempting to get to that level, is the amount of levels you have to test to just get that far. From API server all the way down to state, it's a large task and maybe not worth it?
To correctly parse other values that aren't strings, we should parse via the config using ParseSettingsStrings.
|
|
Merged
jujubot
added a commit
that referenced
this pull request
Jul 8, 2021
#13143 ### Merge 2.9 into develop 6ad00ba (upstream/2.9, origin/2.9, 2.9) Merge pull request #13142 from SimonRichardson/2.8-into-2.9-cache f4b4ab4 Merge pull request #13140 from hpidcock/pull-secrets 185fac6 Merge pull request #13135 from wallyworld/cass-image-repo-port 2b67b0a Merge pull request #13134 from achilleasa/2.9-backport-equinix-provider-support 43b8197 (achilleasa/2.9) Merge pull request #13129 from ycliuhw/fix/lp-1934180 25e8028 Merge pull request #13128 from tlm/oauth-cred-k8s-lp1929910 7b18431 Merge pull request #13133 from SimonRichardson/pod-ready-timeout c139c11 Merge pull request #13130 from SimonRichardson/set-config-empty-string ### Conflicts: CONFLICT (content): Merge conflict in cmd/juju/commands/bootstrap_test.go CONFLICT (content): Merge conflict in cloudconfig/podcfg/podcfg_test.go CONFLICT (content): Merge conflict in cloudconfig/podcfg/image.go CONFLICT (content): Merge conflict in cloud/clouds_test.go CONFLICT (content): Merge conflict in apiserver/facades/client/application/application_unit_test.go CONFLICT (content): Merge conflict in apiserver/facades/agent/uniter/uniter.go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following changes allow the charm config as a map and the charm
config as a YAML to work as intended. The code previously assumed that
there was always a YAML and that the charm config map would always
overwrite it.
The problem occurs when there isn't a charm YAML and it would blindly
overwrite values. This caused empty strings to be converted
to
nil, causing the --reset flow to be triggered. The --resetflow sets the config defaults when it sees nil.
There is a much deeper problem and one that we will need to understand.
API versioning should essentially not touch old code or at the very
least have enough testing to ensure that any new changes keeps the old
behaviour. 100% code coverage in these scenarios would be beneficial to
ensure that we didn't trip up here. The problem with attempting to get
to that level, is the amount of levels you have to test to just get that
far. From API server all the way down to state, it's a large task and
maybe not worth it?
QA steps
Save bundle.yaml
Notice that the default config shouldn't be shown.
Bug reference
https://bugs.launchpad.net/juju/+bug/1934151