Add summary for 'bundle plan' output#3546
Conversation
|
| An attribute name is required after a dot. | ||
|
|
||
|
|
||
| No changes. Your infrastructure matches the configuration. |
There was a problem hiding this comment.
Would it be more informational to show count there as well? and perhaps normalize the message to
with changes:
Plan: 4 to add, 0 to change, 0 to delete, 26 unchanged
without changes:
Plan: 0 to add, 0 to change, 0 to delete, 30 unchanged
There was a problem hiding this comment.
BTW, terraform shows this summary as last line, not first. Makes it easier to see on terminal if you have a lot of output.
There was a problem hiding this comment.
Yeah I like having it at the last line. The problem is just this extra Building python_artifact... line that makes the output slightly messy :-/ That's why I'd propose to do the summary line at the top, signaling a start of the (more) useful output of the command.
There was a problem hiding this comment.
We could separate all logging (there could be more than "Building python artifact") with an empty line on stderr flushed before plan output is printed.
There was a problem hiding this comment.
We could separate all logging (there could be more than "Building python artifact") with an empty line on stderr flushed before plan output is printed.
Yes that could work.
So
random log lines # stderr: lines like "Building Python artifact"
# stderr: newline
create jobs.bar
create ...
Plan: 0 to add, 0 to change, 0 to delete, 30 unchanged
would work as an alternative to the original proposal.
There was a problem hiding this comment.
@andrewnester thoughts on the above summary by @lennartkats-db ? I see plan is still printed before the rest and there does not seem to be separation from other log line.
There was a problem hiding this comment.
I see plan is still printed before the rest
This has changed in the latest commit bcb4da7
| create jobs.bar | ||
| create jobs.baz | ||
| create jobs.foo | ||
| create jobs.independent |
There was a problem hiding this comment.
Why do we need the spaces in front? They make output processing harder, e.g. grep ^delete won't work anymore/
There was a problem hiding this comment.
Two spaces:
- We want some way to distinguish these lines from the heading and from any stderr log lines that we still show (right now just
Building python_artifact...) - Consistency with
cli/acceptance/pipelines/e2e/output.txt
Lines 94 to 95 in 3c03fdd
There was a problem hiding this comment.
We want some way to distinguish these lines from the heading and from any stderr log lines that we still show (right now just Building python_artifact...)
Makes sense. One other thing that can do regardless is insert newline on stderr if there was an output there.
Consistency with
This action will result in the **deletion** of the following Lakeflow Declarative Pipelines along with the
Streaming Tables (STs) and Materialized Views (MVs) managed by them:
**delete** pipeline my_project_pipeline
**delete** pipeline my_project_pipeline_2
Regardless of bundle plan I was thinking of updating this message so that:
- it does not mention delete twice (once in the sentence and one in the line item)
- uses . format that we're adopting everywhere (pipelines.my_project_pipeline).
So it could look like this:
This action will result in the deletion of the following Lakeflow Declarative Pipelines along with the
Streaming Tables (STs) and Materialized Views (MVs) managed by them:
- pipelines.my_project_pipeline
- pipelines.my_project_pipeline_2
cmd/bundle/plan.go
Outdated
| fmt.Println(action) | ||
| } | ||
| } else { | ||
| fmt.Println("No changes. Your infrastructure matches the configuration.") |
There was a problem hiding this comment.
Instead of fmt.Printf and fmt.Println, we should use fmt.Fprintf and friends with cmd.OutOrStdout(). That enables redirection elsewhere and is in line with the rest of the code under cmd. Looking into a linter for this.
There was a problem hiding this comment.
As for the wording here, I suggest:
No resource changes. Your deployment matches the configuration.
Because:
- Code might have changed, and we're not tracking that here
- We don't use "infrastructure" anywhere to refer to a bundle resources
There was a problem hiding this comment.
Yeah it would be good if we consistently show that summary based on the thread above
## Changes Reject directly writing to global stdout/stderr. The "forbidigo" linter includes this rule by default. Also see https://golangci-lint.run/docs/linters/configuration/#forbidigo ## Why The CLI uses Cobra and should therefore use `cmd.OutOrStdout()` and friends. Saw this here: #3546 (comment)
| @@ -1,4 +1,4 @@ | |||
| Local = true | |||
| Local = false | |||
There was a problem hiding this comment.
It's already disabled on main but I guess out.test.toml was not updated there
cmd/bundle/plan.go
Outdated
| fmt.Fprintf(out, "%s %s\n", action.ActionType.StringShort(), key) | ||
| } | ||
| } else { | ||
| fmt.Fprintf(out, "Plan: 0 to add, 0 to change, 0 to delete, %d unchanged\n", unchanged) |
There was a problem hiding this comment.
Should not summary go to stderr? So that you can process the per-resource stats.
There was a problem hiding this comment.
Why is this a branch if the other branch prints the same output?
There was a problem hiding this comment.
@denik I'd prefer users to use JSON output instead of parsing text in such scenarios. Since this command is summary I also prefer summary to be printed to stdout
There was a problem hiding this comment.
@pietern good point, no need to have it anymore
There was a problem hiding this comment.
Having JSON output does not mean we should not get the most value our of text output. grep / sort are much easier than JSON parsing.
There was a problem hiding this comment.
I find it confusing that the plan command shows the summary line in stderr, but the rest of the plan in stdout. I'd prefer to keep all of it in stdout. Parsing text output is error-prone, as we might change this output later with some styling, which can break. Hence, I suggest using JSON for parsing purposes.
cmd/bundle/plan.go
Outdated
| fmt.Fprintf(out, "%s %s\n", action.ActionType.StringShort(), key) | ||
| } | ||
| } else { | ||
| fmt.Fprintf(out, "Plan: 0 to add, 0 to change, 0 to delete, %d unchanged\n", unchanged) |
There was a problem hiding this comment.
Why is this a branch if the other branch prints the same output?
acceptance/bundle/resources/volumes/set-storage-location/output.txt
Outdated
Show resolved
Hide resolved
## Changes Linter will error for switch statements on an enum where not all enum values are covered. The statement can still use a `default:` clause to explicitly fall through. ## Why Observation here: #3546 (comment)
denik
left a comment
There was a problem hiding this comment.
I'd still prefer stdout to be structured even in text mode and all unstructured stuff go to stderr, but it's a minor thing, we can revisit/discuss it later.
## Changes 1. Changed plan output to be printed from stderr (cmdio.LogString) to stdout (fmt.Printf/fmt.Println) 2. Added summary line with counts: Plan: X to add, Y to change, Z to delete Example output: ``` create job.my_project_job delete job.sample_job create pipeline.my_project_pipeline delete pipeline.sample_etl Plan: 2 to add, 0 to change, 2 to delete ``` ## Why Improving the output before making the command publicly available ## Tests Updated acceptance test output Existing acceptance test updated and passing <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
Changes
Example output:
Why
Improving the output before making the command publicly available
Tests
Updated acceptance test output
Existing acceptance test updated and passing