Config upgrade: handle helm overrides#1646
Merged
balopat merged 6 commits intoGoogleContainerTools:masterfrom Mar 15, 2019
Merged
Config upgrade: handle helm overrides#1646balopat merged 6 commits intoGoogleContainerTools:masterfrom
balopat merged 6 commits intoGoogleContainerTools:masterfrom
Conversation
corneliusweig
requested review from
balopat,
dgageot,
nkubala,
priyawadhwa and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1646 +/- ##
==========================================
+ Coverage 46.97% 46.99% +0.02%
==========================================
Files 130 131 +1
Lines 6252 6262 +10
==========================================
+ Hits 2937 2943 +6
- Misses 3014 3016 +2
- Partials 301 303 +2
Continue to review full report at Codecov.
|
added 5 commits
March 10, 2019 22:59
…1585) Helm overrides have type `map[string]interface{}` which cannot be serialized to json. Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com> #####
…tainerTools#1585) Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
…rrides (GoogleContainerTools#1585) Helm overrides have type `map[string]interface{}` which cannot be serialized to json. Implement a custom serialization which includes the overrides as a yaml-string in json. This implementation is forwards-compatible, so that it will automatically work for future apiVersions. Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
corneliusweig
force-pushed
the
config-upgrade
branch
from
1571b57 to
982b421
Compare
Contributor
Author
|
@nkubala Do you think this is a viable approach? Since you commented on the bug report you seem to be the right person to ask that. Besides, this PR has been open for almost a month now, and I would like to resolve this at some point. |
Contributor
|
Thank you @corneliusweig for contributing this! This looks good to me - as in theory this shouldn't break anybody. Let's see integration tests pass and then we can merge it. |
balopat
approved these changes
Mar 14, 2019
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Json serialization does not cope with helm overrides, because its type
map[string]interface{}is not serialized by the json encoder. To deal with that, implement a fallback which includes the helm overrides as a yaml string.That way, the config upgrade is forwards compatible, so that future api-versions do not need to do any manual adaptions for a working json serialization. I also tried a much more manual approach which would have required manual tasks for future api versions 👎
One drawback of my current approach is that it requires a new struct
HelmOverrideswhich is shared among all api-version schemas. However, since the content ofHelmOverridesis opaque to Skaffold anyway, this should be ok.Fixes #1585