-
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
urfave/cli: fix parsing of short opts #1046
Conversation
I'll add a test to |
2c97a1c
to
af24114
Compare
af24114
to
6e7777d
Compare
Vendor an updated version of urfave/cli to fix the parsing of short options. Until the fix is merged upstream, vendor the code from github.com/vrothberg/cli containing both, the latest urfave/cli and the bug fix. Fixes: containers#714 Signed-off-by: Valentin Rothberg <vrothberg@suse.com>
6e7777d
to
0b9ee59
Compare
LGTM |
@edsantiago @mheon @baude PTAL |
LGTM, is there an open PR upstream that we can link to? |
Yes, it's here: urfave/cli#758 |
Of course @edsantiago will find a way to break this again. :^) |
📌 Commit 0b9ee59 has been approved by |
⚡ Test exempted: pull fully rebased and already tested. |
Challenge accepted 😆 |
I've tried:
and it seems that podman now enters an endless loop, I've done a quick test reverting the patch and I don't see the issue anymore. Does it happen for you as well? |
Yes, you're good ;-) |
I'll have a look at it to (hopefully) prepare a patch. |
The bug is here: https://github.com/vrothberg/cli/blob/fix-short-opts-parsing/command.go#L207 Instead of adding the argument, the to-be-translated string is added. |
Vendor an updated version of urfave/cli to fix the parsing of short
options. Until the fix is merged upstream, vendor the code from
github.com/vrothberg/cli containing both, the latest urfave/cli and
the bug fix.
Fixes: #714
Signed-off-by: Valentin Rothberg vrothberg@suse.com