-
Notifications
You must be signed in to change notification settings - Fork 671
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
Add a mode that validates CLI commands/options only #153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any problem with these changes! My only concern is that validation for all commands from the ruby side might take quite a while to run if it has to shell out to this for every command. Though perhaps my understanding of how many times it would have to shell out is incorrect 😄
How many times would the ruby code have to shell out to this, and would it be beneficial to have a way to shell out once with a list of commands that this code could read from?
The only reason I ask is because the gh cli can be a bit slow to start, so if we have to call it a bunch of times, the amount of time waiting for the gh cli to start might make things slower than they need to be
Actually not too, too many times! And this only runs the parsing logic which is relatively quick. In addition, this is planned to be a type of integration test. I didn't really want to deal with parsing my complex JSON structure of commands generated on the Ruby side. Not to mention it's much easier to test pushing from the Ruby gem into this CLI than vice versa. I haven't finished outlining the description in my |
Gotcha, yeah I didn't want it to be a blocker, I just wanted to make note of it. Have you run the process in full yet? I'm curious how long it takes. Obviously it's not going to be a customer-facing process so not a huge deal either way, unless the integration test would take a while |
@luke-engle Check out github/valet#6289 where I have some screenshots of this running. It only takes ~3 seconds or so to run 30 tests. The startup time is relatively negligible since the executable responds quickly and only parses, not runs, commands. Even if we increased the number of test permutations 10 fold, that would only be ~30 seconds or so! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the expected behavior when using/omitting the env var.
❯ TEST_COMMAND_ONLY=true dotnet run --project src/ActionsImporter/ActionsImporter.csproj -- dry-run bamboo build -o tmp -p proj
Valid command!
❯ TEST_COMMAND_ONLY=true dotnet run --project src/ActionsImporter/ActionsImporter.csproj -- dry-run bamboo build
Option '-p' is required.
Option '--output-dir' is required.
❯ echo $?
1
❯ dotnet run --project src/ActionsImporter/ActionsImporter.csproj -- dry-run bamboo build
Option '-p' is required.
Option '--output-dir' is required.
Description:
Target a build plan
Options:
# ...
❯ echo $?
0
Spike results captured internally in another ticket. |
Re-opening this PR, see https://github.com/github/valet/issues/6371#issuecomment-1634620059 for more details. |
0ff952c
to
8cf8abd
Compare
What's changing?
TEST_COMMAND_ONLY
, which only runs the CLI parserOpen to better ENV var name suggestions! Also if there's somewhere else I can put this other than
Program.cs
I'd love to know, as that file is getting somewhat long and complex.How's this tested?
Related github/valet#6371