-
Notifications
You must be signed in to change notification settings - Fork 214
feat(prune): add support for removing charts not tracked in state #4211
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
AustinAbro321
left a comment
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.
Main comment is around prune when entire components are removed. Great start, I think this will be a nice value add
src/pkg/state/state.go
Outdated
| // GetPruneableCharts returns a map of component names to lists of charts that can be pruned, | ||
| // filtered by the provided component and chart names. If componentFilter is empty, all components | ||
| // are searched. If chartFilter is empty, all charts are searched. | ||
| func (d *DeployedPackage) GetPruneableCharts(componentFilter, chartFilter string) (map[string][]InstalledChart, error) { |
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.
Charts are set to orphaned only work when a chart is removed from a component, but not when a component is removed entirely. I believe this could confuse some users. This could also encourage users to add all their charts to a single component so they can use prune in the future if need be.
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.
Could you elaborate more? is this a concern or a required change?
Charts should only be able to be orphaned during deploys. Removes should remove all charts - active or orphaned. I do see room for confusion among the two commands but less of a distinct path forward.
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.
Ah there's a misunderstanding. I mean when a component is removed from one package to another, rather than it being removed with zarf package remove.
For example, if we started with components like so
components:
- name: test-component
required: true
charts:
- name: chart-to-keep
version: 1.0.0
namespace: prune-test
localPath: chart-to-keep
- name: test-component-2
charts:
- name: chart-to-remove
version: 1.0.0
namespace: prune-test
localPath: chart-to-removethen in the next version of the package removed test-component-2. Functionally, I think as a user I'd expect prune to behave the same in both situations, but currently it will only track the chart if it was originally in a component that still exists. In my experience, more often than not a component has a single chart.
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.
Ah interesting - correct me if I understood wrong - not only does Zarf orphan InstalledCharts from state - it also orphans components from state as well?
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.
Yea, when Zarf records a package deployment it overwrites the existing secret and only the components that were in the latest deployment are added to state.
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Description
Adds support for a
zarf package prunecommand. This allows for the explicit removal of a component chart by name OR it can look for all pending/orphaned resources and provide a list. Requires confirmation.Related Issue
Fixes #2992
Checklist before merging