Skip to content

Commit

Permalink
display: only hide replacement steps in diff (#16065)
Browse files Browse the repository at this point in the history
When displaying diff events, we currently try to hide non-logical
replacement steps unless we specifically enable showing them. However we
currently do that for all non-logical operations, regardless whether
they are replacement steps or not.

In particular a RefreshStep is non-logical, but it's also not a
replacement step. We still want to show them during the diff because
their output can be important. Especially if the user just requested a
diff it doesn't make sense to hide the diff from them at the same time.

The intention here is to only hide replacement steps, so do that.

The full diff with the display tests is here:
https://gist.github.com/tgummerer/fcd012f13669a9cdc39530cde7770260 It's
unedited, so it includes some flakyness which isn't interesting.

I looked it over, and I think it looks like what we want, but I'm
curious to hear what others think. E.g.
https://gist.github.com/tgummerer/fcd012f13669a9cdc39530cde7770260#file-testdata-diff-L558
looks more correct now, as it shows the two delete operation that
actually happened, that it didn't show before, and it still shows the
same operation (Calling this one out in particular, since it took me a
bit to understand that we still have the same operation in the diff)

Fixes #7665
  • Loading branch information
tgummerer committed Apr 26, 2024
1 parent bf0aa4a commit 8219c92
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 3 deletions.
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: cli/display
description: Fix output of the diff display, making sure it shows diffs from refreshes
8 changes: 5 additions & 3 deletions pkg/backend/display/display.go
Expand Up @@ -199,10 +199,12 @@ func shouldShow(step engine.StepEventMetadata, opts Options) bool {
return opts.ShowSameResources
}

// For logical replacement operations, only show them during progress-style updates (since this is integrated
// For non-logical replacement operations, only show them during progress-style updates (since this is integrated
// into the resource status update), or if it is requested explicitly (for diffs and JSON outputs).
if (opts.Type == DisplayDiff || opts.JSONDisplay) && !step.Logical && !opts.ShowReplacementSteps {
return false
if !opts.ShowReplacementSteps {
if (opts.Type == DisplayDiff || opts.JSONDisplay) && !step.Logical && deploy.IsReplacementStep(step.Op) {
return false
}
}

// Otherwise, default to showing the operation.
Expand Down
9 changes: 9 additions & 0 deletions pkg/resource/deploy/step.go
Expand Up @@ -1300,6 +1300,15 @@ var StepOps = []display.StepOp{
OpImportReplacement,
}

func IsReplacementStep(op display.StepOp) bool {
if op == OpReplace || op == OpCreateReplacement || op == OpDeleteReplaced ||
op == OpReadReplacement || op == OpDiscardReplaced || op == OpRemovePendingReplace ||
op == OpImportReplacement {
return true
}
return false
}

// Color returns a suggested color for lines of this op type.
func Color(op display.StepOp) string {
switch op {
Expand Down

0 comments on commit 8219c92

Please sign in to comment.