cli: Add initial command views abstraction

[alisdair]

Terraform supports multiple output formats for several sub-commands. The default format is user-readable text, but many sub-commands support a -json flag to output a machine-readable format for the result. The output command also supports a -raw flag for a simpler, scripting- focused machine readable format.

This commit adds a "views" abstraction, intended to help ensure consistency between the various output formats. This extracts the render specific code from the command package, and moves it into a views package. Each command is expected to create an interface for its view, and one or more implementations of that interface.

By doing so, we separate the concerns of generating the sub-command result from rendering the result in the specified output format. This should make it easier to ensure that all output formats will be updated together when changes occur in the result-generating phase.

There are some other consequences of this restructuring:

• Views now directly access the terminal streams, rather than the now-redundant cli.Ui instance;
• With the reorganization of commands, parsing CLI arguments is now the responsibility of a separate "arguments" package.

For now, views are added only for the output sub-command, as an example. Because this command uses code which is shared with the apply and refresh commands, those are also partially updated.

[codecov]

Codecov Report

Merging #27738 (57879bf) into master (04e512d) will decrease coverage by 0.14%.
The diff coverage is 45.05%.

Impacted Files	Coverage Δ
command/meta.go	78.28% <ø> (ø)
commands.go	0.68% <0.00%> (-0.01%)	⬇️
command/views/view.go	17.14% <17.14%> (ø)
command/views/output.go	19.11% <19.11%> (ø)
command/output.go	45.00% <60.00%> (-24.15%)	⬇️
command/apply.go	47.29% <100.00%> (-3.12%)	⬇️
command/arguments/default.go	100.00% <100.00%> (ø)
command/arguments/output.go	100.00% <100.00%> (ø)
command/arguments/view.go	100.00% <100.00%> (ø)
command/meta_config.go	57.32% <100.00%> (+0.55%)	⬆️
... and 11 more

[apparentlymart]

I like this a lot! I've not approved it because I've been in the weeds of this with you a bit this week and so I'd prefer to leave the approval to someone who's coming at it more fresh, but I did want to say that this broadly matches what I was expecting and it's nice to see the new pattern come together in a real example.

I left a few little notes inline but they are more noodles for later 🍜 than blockers for now.

[apparentlymart]

Given how often we seemed to do c.Ui.Output(fmt.Sprintf(...)) I wonder about making this have the signature output(fmt string, args ...interface{}) instead, and then using fmt.Fprintf in here, cause I think in practice that has been the more common case.

Building the formatting into the function might also help nudge towards the more correct interaction with Colorize, where we need to be colorizing the format string rather than the format result in order to avoid interpreting color-sequence-looking things in the format arguments:

view.output(v.colorize.Colorize("Something: [bold]%s[reset]\n"), something)

(As you recently saw, we've got real existing examples of us getting this wrong in the current setup where formatting is separated from output, where the colorize was applied to the formatting result rather than the format string itself.)

---

Separate thought but also related to this particular function:

I guess printing to the output streams is going to be a common enough operation in the views that it might justify having fmt-like helper functions directly on the Streams type for it:

    v.streams.Printf("Something: %s\n", something) // wraps fmt.Fprintf(v.streams.Stdout.File, ...)
    v.streams.Println("Something ", something) // wraps fmt.Fprintln(v.streams.Stdout.File, ...)
    v.streams.EPrintf("Something: %s\n", something) // wraps fmt.Fprintf(v.streams.Stderr.File, ...)
    v.streams.EPrintln("Something ", something) // wraps fmt.Fprintln(v.streams.Stderr.File, ...)

I do want to be cautious about adding too much fluffy abstraction into the terminal package, but given that these are just thin convenience wrappers around an existing abstraction already in the standard library it feels like an okay compromise to me: I think we can assume that most folks who are familiar with Go will understand the analogy to fmt.Printf, fmt.Println, etc (and similar methods in log) and thus not have to think too hard to guess what these functions are going to do.

[alisdair]

I agree with these suggestions, and would like to add more helper functions to prevent the need for views to poke into the streams directly. One concern I have with changing the semantics of output to use a format string is that it is convenient to have a Println equivalent rather than scattering newlines around the call sites. This leads me towards the latter approach you suggested, although I'd like to leave that for a separate PR—this one is already too long.

[apparentlymart]

In the interests of connecting these dots together for potential future reference, we subsequently added these fmt-like functions over in #27759.

[apparentlymart]

It's a shame that the setup here doesn't make it easy to pass in a higher-level mock while testing this layer, so these tests can focus on just the controller and not also implicitly testing one of the views, but I must admit I don't see a clean way of doing it that doesn't either make things much more complex or end up skipping testing the overall command, so this does seem like a good compromise for now. Perhaps as we implement a few more of these we'll start to see some patterns that suggest ways to isolate the controller and the view differently, but this is still a great improvement over what we had before!

[alisdair]

I agree with this being unfortunate. I have an idea for how we might work around this (moving the view constructor to the views.View struct, making views.View an interface, and allowing injection of a call-logging mock as the testView) but it's again not something I want to add this PR.

I would also add that several of the output command tests ought to be moved to the views package, as the controller code coverage is already good enough. Again, I am trying to minimize the diff (although looking at it that might not be obvious) so I want to leave that for a follow-up.

[mildwonkey]

This is fantastic and I love the separation of concern. I do look forward to some of the future PRs that you mention in the comments w/Martin but this is great as is!

I left a teeeeeeeny comment about adding a comment - it's fine if you choose not to, but I found myself briefly thrown by a (completely normal and valid, but not in a place I expected to see it) bit of code. It's not a merge blocker.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.