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

Release 1.21.0 regression when using pass-through commands #850

Closed
coilysiren opened this issue Aug 8, 2019 · 7 comments · Fixed by #872
Closed

Release 1.21.0 regression when using pass-through commands #850

coilysiren opened this issue Aug 8, 2019 · 7 comments · Fixed by #872
Assignees
Labels
kind/bug describes or fixes a bug status/claimed someone has claimed this issue

Comments

@coilysiren
Copy link
Member

In 1.20.0 this used to work

myCLI myCommand --someArg someInput docker run --rm ubuntu bash

In 1.21.0 it's broken, with this error message

Incorrect Usage: flag provided but not defined: -rm

The issue here is that the CLI is reading --rm as input to myCommand, rather than input to docker run.

@coilysiren coilysiren added kind/bug describes or fixes a bug status/confirmed confirmed to be valid, but work has yet to start labels Aug 8, 2019
@coilysiren
Copy link
Member Author

Pass through commands in general are (...) a bit difficult to support. The hotfix here is to invoke docker like this

- myCLI myCommand --someArg someInput docker run --rm ubuntu bash
+ myCLI myCommand --someArg someInput -- docker run --rm ubuntu bash

@coilysiren coilysiren self-assigned this Aug 8, 2019
@coilysiren
Copy link
Member Author

coilysiren commented Aug 8, 2019

Tangentially, when you hit this bug it exits with code 0, which can very easily "gotcha!" you if you're expecting a given command to failed on errors.

EDIT: this is probably a different bug report, though.

@coilysiren
Copy link
Member Author

Ah, the lack of an error response was because my specific CLI wasn't checking for an error from app.Run. I think a previous version of the readme suggested not checking for app.Run errors.

@coilysiren coilysiren added status/claimed someone has claimed this issue and removed status/confirmed confirmed to be valid, but work has yet to start labels Aug 17, 2019
@coilysiren
Copy link
Member Author

I'm going to start looking into this next week 📆

@coilysiren coilysiren changed the title Regression when using pass-through commands Release 1.21.0 regression when using pass-through commands Aug 17, 2019
@coilysiren
Copy link
Member Author

If anyone drops into this issue and definitely knows how to fix the problem, feel free to take this off my hands ^_^ You'll have to race me for the PR though 🏃‍♀

gesellix added a commit to gesellix/couchdb-prometheus-exporter that referenced this issue Aug 24, 2019
Beware before merging: there's an issue in urfave/cli 1.21.0: urfave/cli#850
@coilysiren
Copy link
Member Author

Just ran a bisect, and it looks like this is the commit that caused the bug df562bf

@coilysiren
Copy link
Member Author

Which was merged in this PR #691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug describes or fixes a bug status/claimed someone has claimed this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant