cli: only create resource backups if upgrade is executed #1437
Conversation
✅ Deploy Preview for constellation-docs canceled.
|
Previously backups were created even if no service upgrades were executed. To allow this some things are restructured: * new chartInfo type that holds release name, path and chart name * upgrade execution and version validity are checked separately
derpsteb
force-pushed
the
feat/selective-backup-upgrades
branch
from
112a40c
to
9fb681b
Compare
cli/internal/helm/client.go
Outdated
| var info chartInfo | ||
|
|
||
| switch chart.Metadata.Name { | ||
| case ciliumInfo.chartName: | ||
| info = ciliumInfo | ||
| values, err = loader.loadCiliumValues() |
There was a problem hiding this comment.
I feel like we could skip having to define the chartInfo type if we instead pulled some of the logic that comes after the switch statement into the switch statement.
For example if info == certManagerInfo && !allowDestructive could be moved directly into the cert-manager case.
Similar the call to c.updateCRDs(ctx, chart) can be moved into the the Constellation operators case.
The different cases then only need to set a string for their respective release name, which can be passed into the prepareValues and upgradeAction functions, which are called for all cases
There was a problem hiding this comment.
I applied your suggestions but kept the chartInfo type for now. Reason: I am hoping to unify the loadCilium, loadCertManager, loadOperators, loadConstellationServices in loader.go into a single function, hopefully making the code more concise & understandable. But I would do that in a separate PR to keep PRs small and simple.
If you think this is a bad idea, please tell me. If it turns out to not work as I expect I would remove the chartInfo type again.
There was a problem hiding this comment.
Only allowed for some minor change: #1441
Not sure if it's better that way. "Feels" better because there is slightly less code duplication. I guess the DRY principle sits deep in me :D
But also doesn't make it worse. So 🤷♂️ ..
|
Looks a lot cleaner now and only creates the backups when necessary, nice |
Proposed change(s)
upgrade applyeven if no service upgrades were executed. Now they are only created if at least one service is upgraded. To allow for this change some restructuring was necessary:Checklist