psyinfra / onyo

text-based inventory system on top of git
ISC License
3 stars 5 forks source link

Bug: `--quiet` always requires `--yes` #682

Open aqw opened 1 month ago

aqw commented 1 month ago

Even when it does not make sense, --quite requires --yes. I discovered this while exploring #680

❱ onyo --quiet get --keys path --include shelf
Traceback (most recent call last):
  File "/home/aqw/.venvs/onyo/bin/onyo", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/aqw/git/inm7/onyo/onyo/main.py", line 532, in main
    ui.set_quiet(args.quiet)
  File "/home/aqw/git/inm7/onyo/onyo/lib/ui.py", line 114, in set_quiet
    raise ValueError("The --quiet flag requires --yes.")
ValueError: The --quiet flag requires --yes.

onyo get never prompts the user, and thus there is nothing to say --yes to.

There is a question of what the behavior for other commands should be: 1) maintain present behavior for commands that do prompt the user (check and error on a per command basis) 2) change behavior so that --quiet works even without --yes on commands that prompt; just assume the answer is always no (or skip or whatever is "continue with a no-op").

I also think this error should not include a traceback.

TobiasKadelka commented 1 month ago

It is definitely a bug (how it behaves, but also that there is a traceback), and IMO option 1 is the obvious behavior: to require a --yes when something happens (as an equivalent to --force flags of other commands; a user says they thought about what they are doing), and I would expect onyo to simply not require a --yes flag for non-prompting commands if there is nothing to agree to.

Doing anything more clever seems like dangerous (and needlessly confusing) territory, even if it is just assuming "no" as a response (which renders all commiting commands useless anyway?).

aqw commented 1 month ago

I'll rephrase my question a bit. We have a choice about how --quiet without --yes behaves (for interactive commands):

1) exit 1 immediately with error message (a bit ironic for --quiet) 2) attempt to execute everything, but without printing messages (indeed, quiet), and always answer the default to prompts (which should all be no-op; we need to audit these). If the command would succeed, then exit 0. If an error is encountered, then exit 1.

To me, option 2 sounds more correct.

TobiasKadelka commented 1 month ago

I disagree and favor 1.

The main situation in which I want to use --quiet is scripting, not the terminal (where I want to see what I am doing and how it effects the repository).

When I execute a script (lets say the demo repository) and we would go with option 2, I want to still see errors, especially when any command does not execute, i.e. onyo move does not move, onyo rm does not remove.

If your idea is to go with "default" when using --quiet without --yes (which is most of the time "yes" IIRF, but either way), then I am strongly against that to. IMO that would mainly just render the --yes flag obsolete in most situations, and I do think it should exist (mainly for scripting, but also in terminal).

In summary, I think the ideal and expected behavior is:

  1. Require --yes for --quiet usage for all commands that change the repository, if it is not there, error.
  2. Don't require --yes for commands (e.g. onyo get) that do not change the repository ever, allow it.
  3. (remove the showing of the traceback)
aqw commented 1 month ago

I see the scripting use-case somewhat differently. I may want to test if something will work in principal, but have it exec no-op (i.e. without --yes) and without noise (i.e. --quiet). I'm just running it to see if it would error.

For example, (completely contrived):

key_list = "cpu.arch display.resolution model.name"
# test if all keys are kosher
for k in $key_list ; do
    onyo --quiet unset --keys $k || echo "$k cannot be unset"
done

A less contrived example would attempt to set new values in a bunch of assets, and see if they pass validation (still to come), before actually proceeding with the commits.

As always, there's alternate ways to do these things, and there's only so much you can simulate in advance. But the idea of a test run is not entirely uncommon.

I do see the argument that --quiet only implicitly puts us in --dry-run mode. But I do think --quiet has valuable in both --yes and (in the hypothetical) --dry-run modes.

TobiasKadelka commented 1 month ago

Yeah, that is indeed a valid and useful use-case. And while reading your message, even before you said it yourself, everything in me was shouting --dry-run. I also think a --dry-run flag should be a seperate functionality, because it says explicitely and obviously what will happen, without the need to explain why --quiet would behave in that manner.

So basically, I agree about the use-case, and I also still think exactly the same about possible behaviors of --yes and --quiet like I stated above. And there should be an issue for a seperate --dry-run flag, because I think there are more dangers in overcomplicating these very simple flags if they can work just as well in the simple and obvious way :)

aqw commented 1 month ago

Ben and I talked about this a bit, and I think we will defer the talk about --dry-run etc until later.

He's long felt that --yes should rather be --non-interactive, and then there's --dry-run. And he's also had ideas for allowing users to format the output of commands (ala DataLad), which intersects with --quiet vs --silent, etc, etc. It's all connected. See also #497.

So this bug should remain focused on simply moving the "yes" vs "quiet" check to the commands that require it.

TobiasKadelka commented 1 month ago

Thanks for the update!

simply moving the "yes" vs "quiet" check to the commands that require it.

That is the most important part of this issue anyway. The behavior you found is one of the things that change not much, but it would annoy me every time just a tiny bit when it happens :P

aqw commented 4 weeks ago

I started working on this, and in the end, solutions either introduced hacks or new structures to solve this in a reasonable way.

IMO, this indeed blocks on #497

TobiasKadelka commented 4 weeks ago

Thanks for the update! I still think this (as well as #497) should happen anyway.

Is your conclution to not do them both, or just that it is more tricky/time consuming?

aqw commented 4 weeks ago

Both definitely need to happen (along with additional overall work around controlling output). Just that 90% of this issue is easy, and the last bits touch exactly the problems that we have overall with messaging.