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

Improve cli #3386

Open
11 of 28 tasks
anbraten opened this issue Feb 13, 2024 · 10 comments
Open
11 of 28 tasks

Improve cli #3386

anbraten opened this issue Feb 13, 2024 · 10 comments
Labels
cli summary it's a summary for lot of issues

Comments

@anbraten
Copy link
Member

anbraten commented Feb 13, 2024

The woodpecker cli could get some polishing:

@anbraten anbraten added cli summary it's a summary for lot of issues labels Feb 13, 2024
@xoxys
Copy link
Member

xoxys commented Apr 28, 2024

@woodpecker-ci/maintainers I would like to work on improve error handling for missing args (missing repo id / name results in an parseInt error atm) How do we want to handle it? Do we really want to continue using args? IMO, it would be better to drop args completely and just use flags. Any reason to keep using args?

@anbraten
Copy link
Member Author

anbraten commented May 15, 2024

I think its quite common practice to use args (see kubectl, docker, ...) therefore I would suggest to stick to it and add an empty-string check with a more descriptive error in front of the parseInt call.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

Obviously, but I could also name a lot of tools without args. We could have a way more clean code without arg validation, as flag validation and required flags is a built-in feature of urfave/cli. However, if args is preferred, that works for me as well.

@lafriks
Copy link
Contributor

lafriks commented May 15, 2024

I don't know if urfave/cli still checks that required arguments are not empty if required (you can provide argument but provide it with empty value) so check for empty value could still be needed

@xoxys
Copy link
Member

xoxys commented May 15, 2024

There is no required args feature, just a required flags option.

@jinnatar
Copy link

While the new setup flow seems nice, it utterly fails on a headless machine. As a quick fix I'd recommend echoing the URL that it's trying to apparently open in a browser or otherwise provide a flow where I can open the URL on another device and feed back a token on the cli.

@qwerty287
Copy link
Contributor

You can easily set URL and token with the WOODPECKER_SERVER and WOODPECKER_TOKEN env vars in these scenarios.

@jinnatar
Copy link

You can easily set URL and token with the WOODPECKER_SERVER and WOODPECKER_TOKEN env vars in these scenarios.

The problem is the setup just "stops" when the browser open is attempted behind the scenes. If it was a bit more verbose about it, wouldn't be as confusing.

@qwerty287
Copy link
Contributor

qwerty287 commented Aug 25, 2024

Yes, the log message should print that you can use the env vars. I put it on the list above.

@6543
Copy link
Member

6543 commented Aug 25, 2024

yes we should check if the env has an wayland or xorg session, and if not we print out this sugestion and fail as we run "headless" ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli summary it's a summary for lot of issues
Projects
None yet
Development

No branches or pull requests

6 participants