feat: send untouched resource info to legacy progress reporter - #664
feat: send untouched resource info to legacy progress reporter#664dmmordvi wants to merge 15 commits into
Conversation
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…kipped Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
dmmordvi
left a comment
dmmordvi
left a comment
There was a problem hiding this comment.
Code review (3 findings, all MINOR — no blockers). The PR is correct and ready to merge; the two potentially serious points below are accepted as deliberate.
Findings not anchorable to the diff:
pkg/plan/legacy_progress_reporter_ai_test.go:— No test covers the real main→failure/rollback scenario where the full instResInfos is re-passed as untouched into a second StartStage; only the nil-untouched case is covered by TestAI_StartStage_UntouchedScopedToStageAndFrozen. Recommend adding a scenario test that pins the expected re-emission behavior so regressions become visible. (generated by pi-pi)
| LegacyProgressReporter: opts.LegacyProgressReporter, | ||
| TrackingOptions: opts.TrackingOptions, | ||
| NetworkParallelism: opts.NetworkParallelism, | ||
| InstallableResourceInfos: installableInfos, |
There was a problem hiding this comment.
The full original installableInfos is passed into the failure plan, so every install resource still present in the cluster is re-emitted as NoOp/Completed in a stage where it is not an operation of that plan. This diverges from the StageReport doc contract ("ALL operations in the plan"). If this is intended (giving the consumer the full picture), consider clarifying the StageReport doc comment to mention these NoOp pseudo-operations.
(generated by pi-pi)
Add TestAI_StartStage_UntouchedReemittedAcrossStages, which drives startStage twice with the same non-empty untouched inventory (modeling the full instResInfos passed into a second stage on main→failure/rollback). Verifies the unchanged resource is re-emitted as Completed/NoOp in the new active stage, retained in the frozen prior stage, and not duplicated when its ObjectRef matches a plan operation of the second stage. Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
… skipped Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
dmmordvi
left a comment
dmmordvi
left a comment
There was a problem hiding this comment.
Code review pass (pi-pi). One new MINOR finding below. A prior review already flagged the OperationTypeNoOp wire-compatibility concern on pkg/legacy/progrep/progress_report.go:16, so it is not re-posted here.
| }) | ||
| } | ||
|
|
||
| for _, info := range untouched { |
There was a problem hiding this comment.
A resource with real drift can reach this NoOp/Completed branch. resourceInstallType() returns ResourceInstallTypeNone not only when the live and dry-apply states match, but also when a resource policy (skip-update / skip-create / skip-recreate) forces None despite an actual diff (skippedByPolicy=true). Such a resource produces no plan operation (and mustTrackReadiness is false), so it flows into this loop and is reported as NoOp/Completed. That reads as "unchanged" to the user even though the resource actually differs from the desired state and was only skipped by policy. This is display-only in the legacy progress reporter (deploy behavior is unaffected), but the label is misleading; consider distinguishing "skipped by policy" from "unchanged".
(generated by pi-pi)
There was a problem hiding this comment.
We return NoOp type, so it is enough
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
1 participant
Uh oh!
There was an error while loading. Please reload this page.