Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rush] Add "suppressStartupBanner" flag for CLI actions #2777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmichon-msft
Copy link
Contributor

Summary

Adds an option suppressStartupBanner that can be specified in command-line.json to skip logging of Rush's startup banner, e.g. if the command output is meant to be parsed as JSON or piped to some other tool.

Details

Changes the startup banner to be written to a string and defer printing to command line action invocation.
Suppresses logging from install-run-rush.ts unless --debug or -d are passed.
Fixes #2607

How it was tested

Ran various local commands and verified presence or absence of the banner.

@@ -23,6 +23,7 @@ export class CheckAction extends BaseRushAction {
"Checks each project's package.json files and ensures that all dependencies are of the " +
'same version throughout the repository.',
safeForSimultaneousRushProcesses: true,
suppressStartupBanner: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think this command should never print the startup banner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The startup banner is noise that reduces the clarity of the error. Ideally we'd implement alternative reporters for CI such that we could, e.g. use Azure DevOps logging commands to specify exactly the problem (instead of just exiting with code 1), but that's not really in scope here. Currently rush check having the startup banner is a nuisance.

@@ -34,7 +34,8 @@ export class ListAction extends BaseRushAction {
'List package names, and optionally version (--version) and ' +
'path (--path) or full path (--full-path), for projects in the ' +
'current rush config.',
parser
parser,
suppressStartupBanner: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the startup banner only be suppressed if the --json flag is passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to be able to use rush list --to PROJECT | ForEach-Object { # Do stuff } in PowerShell, instead of rush list --to PROJECT --json | ConvertFrom-Json | % { # Do stuff with $_.name }. More importantly, bash or batch scripts can work well with the former but not the latter.
Much of the intent of this feature was to avoid the "only suppressed if --json flag passed" logic and make it be the entire command having that decision.

@elliot-nelson
Copy link
Collaborator

elliot-nelson commented Oct 9, 2021

Was talking with @octogonz about this feature yesterday.

If I were to list out some requirements, I think I'd include:

  1. Users should be able to create custom parameters that suppress banners (like --json or --csv).
  2. Rush built-in commands should be able to support parameters that suppress banners (like --json or --csv).
  3. Banners are never printed in certain situations (for example, tab-completion).
  4. Banners should always be printed in certain situations (for example, -h/--help).

And then some nice-to-haves:

  1. Users should be able to create custom commands that always suppress banners.
  2. Rush built-in commands should be able to always suppress banners.

The reason I think (1) and (2) are more important than (5) and (6) is because a parameter is able to show intent -- "suppressing a banner" is a little arbitrary, whereas a parameter like --json or --csv or perhaps --text is actually specifying an API -- you're indicating what machine-readable format will be output, which requires not including extra lines of text.

If we assume that all Rush commands are by default intended for humans, then we probably shouldn't suppress banners for any built-in commands... for i in $(rush list) might work today, but not tomorrow, because we might adjust the formatting of that command to be prettier. I think it would be safer to force script authors to use a specific output flag, like --json, for scripting. (Perhaps we could add a rush list --porcelain or rush list --text to indicate a text-based, script-friendly output, that won't be adjusted in the future.)

I would make the same argument for user commands, except that forcing a user to add a parameter to show intent requires them to make a custom parameter, add associatedCommands, to it, etc., which seems needlessly pedantic. So we probably should support user-created "always suppressed" commands.

One possible suggestion for the interface could be to add a section to command-line.json:

{
    "suppressBannerOutput": {
        /**
         * If any of these parameters are specified on the command line for any command, startup banners
         * will automatically be suppressed.
         */
        "parameters": ["--json"],

        /**
         * These commands will automatically suppress the startup banner when run.
         */
        "commands": ["my-custom-command"]
    }
}

(This would allow the user to suppress startup banners for rush list in their own monorepo if they desired, even if we don't recommend it. It also allows them to extend what today is a "magic" --json flag to include their own common scriptable formats, like --csv or --xml.)

In the future, we might even add an "always": true option to this configuration section, for a user who never wants to see a Rush startup banner.

@octogonz
Copy link
Collaborator

a parameter like --json or --csv or perhaps --text is actually specifying an API -- you're indicating what machine-readable format will be output, which requires not including extra lines of text.

I'm not sure I like the name --text. The names --json and --csv clearly indicate a machine-readable format, whereas --text could be misinterpreted as merely "no color codes" or "no table formatting".

Also, JSON and CSV seem like special "recommended" formats:

  • JSON: generally recommended for machines -- Node.js scripts can parse it natively, and new fields can be added without breaking compatibility.
  • CSV: generally recommended for humans -- you can open this file in Excel for sorting/filtering/slicing, and you can search it using grep because item fields appear on the same line. Note that correctly escaped CSV requires a somewhat complex NPM package to read/write. (A naive approach like array.map(x => JSON.stringify(x)).join(',') will mishandle punctuation and whitespace characters.)

(Maybe YAML could also be lumped in there, if you want comments in your output. Although none of the popular NPM packages for writing YAML seem to actually support comments.)

So perhaps all the other non-recommended formats could be grouped under a single parameter like --serialize=yaml or --serialize=text? I chose the name "serialize" because it clearly suggests that another program is supposed to be able to deserialize the output, but feel free to suggest something else.

@octogonz
Copy link
Collaborator

BTW @elliot-nelson in this discussion we've been assuming that the motivation for suppressing the banner is to enable deserialization of the output. A different motivation would be if someone simply finds the banners to be too noisy. So we could imagine a --quiet switch that also suppresses the banner, but with less guarantees about the output tidiness.

And speaking of guarantees: If the tool aborts with an error, ideally --json mode should emit the error message in JSON format.

@dmichon-msft
Copy link
Contributor Author

@octogonz , @elliot-nelson , I'm pretty much at the point where I would rather only display the banner when --help (or maybe --debug) are passed, and suppress it the rest of the time. Rush adding boilerplate to custom commands is a nuisance more than a help. Similar to the version-selector code in Heft (which we should add a separate bin entry for that doesn't run the version selector).

@elliot-nelson
Copy link
Collaborator

@dmichon-msft @octogonz I guess I'd hate to lose the "normal" banners all at once just because it's too complicated to hide them some of the time.

Maybe the first step is a lot simpler than either PR idea -- just a "rush --quiet list"?

That seems like it satisfies almost all the would-be script and pipeline uses of rush commands, including built in and custom. And would probably be quite easy to do (most of the work is in this PR).

@M3kH
Copy link

M3kH commented Dec 13, 2021

I don't want look too pushy, but just in case it might be useful, there is an API from nodejs for having debug output: https://nodejs.org/api/util.html#utildebuglogsection-callback and looks that the banner and other thing might use that in combination with the already existing env NODE_DEBUG.

Hope it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[rush] When passing --json install-run-rush should not output extra information
5 participants