feat: add source and package status to package state - #5007
Conversation
Signed-off-by: Wayne Starr <me@racer159.com>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Wayne Starr <me@racer159.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Signed-off-by: Wayne Starr <me@racer159.com>
There was a problem hiding this comment.
Code & Security Review
Adds Source and an overall package Status to the in-cluster DeployedPackage state, surfaces them in zarf package list, and threads status transitions through deploy/remove. Clean, well-scoped, and backwards-compatible (omitempty + GetStatus() → Unknown for pre-existing secrets, reflected in the updated testdata). e2e coverage for the new fields is solid.
Verified locally on Go 1.26.4:
gofmt -lclean, andgo build+go vetpass on the changed packages (src/pkg/packager/...,src/pkg/state/...,src/cmd/...). The full test suite (incl. e2e, which needs a live cluster) is left to CI.
Critical Issues
None.
Security Review
Strengths:
- The recorded
Source(oci:// URL or tarball path) is not credential-bearing for Zarf, and it is stored in the samezarf-namespace package secret that already holds deployment metadata — no new exposure surface. - No injection / auth / crypto surface touched; changes are pure state plumbing.
Concerns: None.
Code Quality
🟡 src/pkg/packager/remove.go:219-225 — partial-remove hardcodes Status = Succeeded, which can misrepresent package health.
After a successful partial remove (some requested components removed, others left untouched), the package status is unconditionally set to PackageStatusSucceeded. The remaining components are never re-evaluated, so a leftover component that still carries ComponentStatusFailed (e.g. removing a healthy component out of a previously-Failed deploy) leaves the package reporting Succeeded while a child component reports Failed. That internal inconsistency is exactly what the new Status column is meant to surface in zarf package list. Consider deriving the reset value from the remaining DeployedComponents (or only resetting to Succeeded when all remaining components are Succeeded, else Failed/Unknown).
🟢 src/pkg/packager/remove.go — status writes guard on opts.Cluster != nil rather than requiresCluster.
In the cluster != nil && !requiresCluster path, the else branch (lines 98-105) builds a fresh depPkg with only component names — no Source, Digest, Generation, or InstalledCharts. The new UpdateDeployedPackage calls (lines 135-140, 198-202, 212-225) then apply that stripped object over the real stored secret. This is largely pre-existing (the inner chart-removal updates at the old 165/187 already did this), but the added writes propagate the clobber. Worth confirming whether these status writes should also gate on requiresCluster for consistency.
🟢 src/cmd/package.go:1330-1338, 1377 — Digest and Source are emitted in JSON/YAML list output but omitted from the table columns (only Status was added to the table header). Presumably intentional to keep the table narrow; flagging the JSON-vs-table asymmetry in case it's unintended.
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Request Changes — driven by [0 Red, 1 Yellow, 2 Green] findings. The Yellow (partial-remove status accuracy) directly undercuts the observability goal of this feature; addressing it in this round avoids a follow-up.
There was a problem hiding this comment.
Commenting some ideas, but I think this should be hashed out further in a ZEP. Can be a short one, but I'd prefer that we have a document to gather feedback from others, especially since this data is largely targeted at those integrating with Zarf.
For instance #4969 was another recent request for additional data from state
| ValuesOverridesMap ValuesOverrides | ||
| // Source is the original source string used to load the package (e.g. oci:// URL or tarball path). | ||
| // Recorded in the cluster's deployed package state. | ||
| Source string |
There was a problem hiding this comment.
From a consumer perspective, will you care about the actual source or only which type of source. If the latter, then an enum here might be better
There was a problem hiding this comment.
Actual source - tarballs are hard to reconstruct but an oci:// reference for example could be repulled and deployed again if you needed to.
| } | ||
|
|
||
| // PackageStatus defines the overall deployment status of a Zarf package. | ||
| type PackageStatus string |
There was a problem hiding this comment.
I'm curious what you'd think of splitting out the state of the lifecycle with the health of the package. Something like below, this could also apply to components, though we'd want to keep their current status for some time for backward compatibility
// PackagePhase is the current lifecycle operation.
type PackagePhase string
const (
PackagePhaseIdle PackagePhase = "Idle"
PackagePhaseDeploying PackagePhase = "Deploying"
PackagePhaseRemoving PackagePhase = "Removing"
)
// PackageHealth is the outcome of the most recent completed operation.
type PackageOutcome string
const (
PackageOutcomeUnknown PackageOutcome = "Unknown"
PackageOutcomeHealthy PackageOutcome = "Healthy"
PackageOutcomeDegraded PackageOutcome = "Degraded"
)
Copy - thinking of resurrecting the old zarf-dev/proposals#27 ZEP for this - would it be "ok" to consider Zarf behavior changes there as well? (i.e that if a component (I think that can be done without a full history system, just look at the old state if the package requires the cluster and reconcile and the more I think about it I don't know if we need a full history system either but we can discuss that too) |
|
@Racer159 Yeah I think it makes sense to resurrect that ZEP |
Resurrected here zarf-dev/proposals#27 |
4 participants
Description
This adds the package's source and an overall package status to Zarf's DeployedPackage in-cluster state.
Related Issue
Fixes #4182
Checklist before merging