[#6097] improve(CLI): Add --quiet option to the Gravition CLI

[Abyss-lord]

What changes were proposed in this pull request?

Add a --quiet option to the Gravition CLI to suppress all output except for errors. The following command implements the logic for processing the --quiet argument.

1. AddXXX
2. CreateXXX
3. XXXEnable
4. XXXDisable
5. DeleteXXX
6. GrantXXX
7. RemoveXXX
8. RevokeXXX
9. SetXXXProperty

All tests have been updated. Currently, #6139, #6147, and #6152 have not been merged yet. I will update the XXXCommandHandler after they are merged.

Why are the changes needed?

Fix: #6097

Does this PR introduce any user-facing change?

No

How was this patch tested?

local test + UT

gcli metalake update -m demo_metalake --disable -i
#demo_metalake has been disabled.

gcli metalake update -m demo_metalake --disable -i --quiet

gcli metalake update -m demo_metalake --enable -i
# demo_metalake has been enabled.

gcli metalake update -m demo_metalake --enable -i --quiet

[Abyss-lord]

@justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback.

[justinmclean]

In general, this looks good, and only a couple of minor comments, but I'm not sure why you added eq calls to many tests when it's not required.

[Abyss-lord]

Hi @justinmclean , I’ve finished updating the code. Please take a look at the PR again when you have time.

[justinmclean]

Looks good, just one comment to address that was unresolved.

[Abyss-lord]

Looks good, just one comment to address that was unresolved.

@justinmclean fix, plz review again.

[tengqm]

/lgtm

[Abyss-lord]

@justinmclean @jerryshao I’ve finished updating the code. Please take a look at the PR again when you have time.

[justinmclean]

LGTM, thanks for your work here.

[jerryshao]

What is the usage of this variable, can we elaborate the usage scenario for this? Also the change seems too fragmented, we need to change the code everywhere, A better way is to define an output class and can be controlled in there, rather than adding system output everywhere.

Besides, if we don't want output, we can simply use /foo/bar 2&1 > /dev/null, which is a typical usage pattern in *nix, I'm curious what is the benefit of adding this.

[shaofengshi]

What is the usage of this variable, can we elaborate the usage scenario for this? Also the change seems too fragmented, we need to change the code everywhere, A better way is to define an output class and can be controlled in there, rather than adding system output everywhere.

Besides, if we don't want output, we can simply use /foo/bar 2&1 > /dev/null, which is a typical usage pattern in *nix, I'm curious what is the benefit of adding this.

+1; the current implementation is not good.

[justinmclean]

It was added at the request of @tengqm with the advice of https://clig.dev.

For example: "For instances where you do want no output (for example, when used in shell scripts), to avoid clumsy redirection of stderr to /dev/null, you can provide a -q option to suppress all non-essential output."

[tengqm]

It was added at the request of @tengqm with the advice of https://clig.dev.

For example: "For instances where you do want no output (for example, when used in shell scripts), to avoid clumsy redirection of stderr to /dev/null, you can provide a -q option to suppress all non-essential output."

Right. This change seems like a compromise between stricter traditional command outputs and something advised elsewhere. In some reviews to CLI command outputs (cannot recall exactly which one though), I raised my concern about the necessity of printing success messages, such as "something has been successfully removed/updated". I was more inclined to explicit, ationable error messages when things go bad, or silent success with no output if an acction succeeded. We are not creating a chatbot with the CLI tool after all. Verbose outputs may also complicate the scriptability of the CLI command. There were other arguments that this verbose messages are actually desirable, and they can be suppressed using a -q argument. I tend to agree with this approach. That is the background.

Back to the implementation, I am a little bit sick of the actual implementation at this moment. Without an appropriate schema to group or to abstract the command line arguments, this kind of changes won't be uncommon in the foreseeable future. For example, for all list operations, we may want a --sort option, or even a --sort-order option; for another example, we may want filtering, paging options.

A cleaner solution, on top of my head, is to have something like this:

• a GenericOptions for flags like url, metalake, authenticate, ignore-version, verbose etc.
• a ListOptions for flags like sort, output-format, filter, ...
• a DeleteOptions for flags like force, cascaded, forever, etc.

We are not there yet. But if we are on the same page, knowing where we are heading, this change should be fine.

[justinmclean]

I think one small improvement would be to add this to the base class:

  protected void optionalOutput(String message) {
    if (!quiet) {
      System.out.println(message);
    }
  }

and call optionalOutput in commands to remove the multiple if (quiet) return; lines. This gives more flexibility in the future regarding the control of the output.

Another solution to avoid passing quiet around (and changing so many files) would involve creating a global config singleton, but that has its own issues and I think is to be best avoided. We could also pass around a command context (same no of files changed, but less changes would be required for future similar changes) or as suggested above grouping different options into classes. I would suggest doing that as a separate PR for this, as the saying goes "Make it work, then make it good."