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

systemctl: hide legends with --quiet, allow overriding #18596

Merged
merged 6 commits into from Feb 17, 2021

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Feb 15, 2021

This makes --quiet imply --no-legend, but allows --legend to be used to
override that. --quiet controls hints and warnings and such, and --legend controls
just the legends. I think it makes sense to allow both to controlled independently,
in particular --quiet --legend makes sense when using systemctl in a script to
provide some user-visible output.

Fixes #18560.

I think we would want to provide the same interface in other tools. I'll
convert them if there's agreement on the approach.

@yuwata
Copy link
Member

yuwata commented Feb 15, 2021

Looks nice!

@yuwata yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Feb 15, 2021
Copy link
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

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

Makes sense to me as a way to handle this combination, looks nice

man/standard-options.xml Outdated Show resolved Hide resolved
src/systemctl/systemctl.c Outdated Show resolved Hide resolved
@yuwata
Copy link
Member

yuwata commented Feb 15, 2021

BTW, it is welcome if shell completions for systemctl are updated.

@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Feb 15, 2021
@keszybz
Copy link
Member Author

keszybz commented Feb 15, 2021

Updated.

  • shell completions as adjusted, see commit message for details.
  • resolvectl and systemctl now acccept --legend[=BOOL].
  • I added a few preparatory patches ;)
  • rebased

Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

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

looks good, except for the optional_argument thing

src/shared/parse-argument.c Outdated Show resolved Hide resolved
src/systemctl/systemctl.c Outdated Show resolved Hide resolved
@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 16, 2021
@keszybz keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 17, 2021
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

LGTM. But one minor comment.

man/resolvectl.xml Outdated Show resolved Hide resolved
@keszybz keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Feb 17, 2021
} else
arg_man = true;

r = parse_boolean_argument("--man", optarg, &arg_man);
Copy link
Member

Choose a reason for hiding this comment

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

hmm, first argument should probably be suffixed with "=", it's a switch that expects an argument after all, and the old error message did suffix it like that.

i.e. we usually have this rule to suffix cmdline switches and config settings that take an argument with "=", to indicate that.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I added this now in the places where the arg is required (but not where not).

src/analyze/analyze.c Show resolved Hide resolved
@poettering poettering added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Feb 17, 2021
This nicely covers the case when optarg is optional. The same parser can be
used when the option string passed to getopt_long() requires a parameter and
when it doesn't.

The error messages are made consistent.
Also fixes a log error c&p in --crash-reboot message.
…n type

This still works nicely, but we need to assign the return value ourselves.
As before, one nice effect is that error messages are uniform.
…usion

We were passing a reference to 'int arg_seal' to config_parse_bool(),
which expects a 'bool *'. Luckily, this would work, because 'bool'
is smaller than 'int', so config_parse_bool() would set the least-significant
byte of arg_seal. At least I think so. But let's use consistent types ;)

Also, modernize style a bit and don't use integers in boolean context.
--no-legend is replaced by --legend=no.

--quiet now implies --legend=no, but --legend=yes may be used to override that.
--quiet controls hints and warnings and such, and --legend controls just the
legends. I think it makes sense to allow both to controlled independently, in
particular --quiet --legend makes sense when using systemctl in a script to
provide some user-visible output.

Fixes systemd#18560.
I don't think it makes sense to complete --legend=yes. It is the default, and
it would be only used very rarely (and then it is easy enough to just remove
the '=no' part from the suggested string).
@keszybz keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Feb 17, 2021
@poettering poettering merged commit dc288ff into systemd:main Feb 17, 2021
@keszybz keszybz deleted the systemctl-quiet-legend branch February 18, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
busctl good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed journal journal-remote resolve systemctl
Development

Successfully merging this pull request may close these issues.

--failed and --quiet is not so quiet
4 participants