-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Quadlet -dryrun arg #16926
Quadlet -dryrun arg #16926
Conversation
cmd/quadlet/main.go
Outdated
if err != nil { | ||
Logf("Can't create dir %s: %s", outputPath, err) | ||
if len(units) == 0 { | ||
Debugf("Not files to parse from %s", sourcePaths) |
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.
Should this be No files to parse ...
Any chance you can add a test? Otherwise add [NO NEW TESTS NEEDED] to the commit message above. |
Your new test is failing. |
Is this a CI issue? The following test also failed: |
Yes just concentrate on fixing your tests. the other failures are almost guaranteed to be flakes. |
LGTM once the tests are all hip. |
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.
Changes LGTM
Please squash the commits into one before merging.
Signed-off-by: Leonardo Rossetti <lrossett@redhat.com>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: odra, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
This seems to be a very odd choice ... isn't this option normally written |
Not in the world of Go. The entire Go toolchain uses only one dash for CLI options. Using two would imply using a different CLI parses and in turn binary bloat. |
Exactly my argument ... Podman/Quadlet is not a piece of the Go toolchain and has a large body of already implemented CLI options that follow a different naming scheme ... hence this choice is inconsistent in the context of Podman /Quadlet. Ironically even the author of #15903 assumed this option should/would be written I didn't want to drag this out, I just wanted to point out this inconsistency before it gets "set in stone" by the upcoming 4.4 release and trips up users, maintainers and documentation writers down the line. 😓 |
Signed-off-by: Leonardo Rossetti lrossett@redhat.com
I've added a
-dyrun
argument to the Quadlet CLI so generated unit files are printed to stdout instead of creating a file - I found this useful when I needed to check how the end result of the generated unit file.Does this PR introduce a user-facing change?