-
-
Notifications
You must be signed in to change notification settings - Fork 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
cmd: Migrate to spf13/cobra
, remove single-dash arg support
#4565
Conversation
332487b
to
d33305d
Compare
Worth noting this could be a breaking change for anyone who happens to use
But this is also a benefit in the sense that we could now provide single letter shorthands for some flags. |
The single-dash flags seem to be already used and referenced across the interwebs, examples:
This PR introduces a breaking change to the CLI. Scripts and systemd units using the single-dash flag will break and stop working. There doesn't seem a way to do this in backwards compatible manner, unless I'm missing something. |
Thanks for working on this Mohammed. It's been a while since I looked at the manpages thing; is the main advantage of this approach over caddyserver/dist#73 the support for multiple shells? Given the extensive changes needed and the single-hyphen thing, I am not sure if it is worth it at this point... which is a bummer to say, but... what do you think? |
I'd love to get shortcuts for flags so we could do stuff like I personally think the breakage is worth it for the UX benefits. The help docs will be normalized nicely (no longer confusingly use This is also overall less code, because some stuff we did ourselves is handled by coral automatically already (like the help command). But we do need to make it clear in the release notes when this goes out that this is a breaking change for those that do use the |
The capability to generate, correct, and consistent shell auto-completion scripts is a pretty major gain, IMO. Francis and I worked on the auto-completion support (caddyserver/dist#33 and caddyserver/dist#35), and it's quite a churn to keep the completion scripts and the caddy commands/flags in sync. Note how we haven't updated it recently, and it's missing the Not to mention none of the available external tooling for generating shell-completions is perfect, each has its own quirks, and I had to make few compromises along the way when I surveyed the scene. Moreover, the current approach to generate the completion script does not accommodate commands or flags plugged by non-standard caddy modules. Coral will always generate consistent and complete shell-completion scripts. I'm not a fan of the breakage, but the maintainability gains are a big plus. We need to communicate the breakage well enough for everyone to validate their scripts. |
cmd/caddy/main.go
Outdated
caddycmd.Main() | ||
caddycmd.Execute() |
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.
Leaving a note for self when I come back to this later: changing Main to Execute this breaks xcaddy because the template uses caddycmd.Main()
. That said, .Execute()
doesn't have to be .Execute()
. Whatever .Execute()
does, .Main()
can do, and we just keep .Main()
and not have .Execute()
.
As of Cobra v1.4.0, there is not dependency on Viper, which thins its dependency to minimum and matches coral, so swapping coral for cobra does not introduce any new deps (across the two). |
This is looking good. I like the smaller dep tree with Cobra 1.4 as well. Just going to pull it down and spin it around before I review it! |
I just realized that single-hyphen flags were documented when using the standard Go Actually maybe it's fine, since our website docs use We might need to merge this in after a big release that explicitly notes that |
spf13/cobra
, remove single-dash arg support
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.
This is very cool, thanks. Finally pulled it down and tried it out. I like that it's fairly minimal change, not very disruptive!
Approving, but I have a couple minor nits (see other comments)
Kind of a bummer that we have 2 markdown libs in our dependencies now, since we use goldmark and cobra uses blackfriday. Oh well. Nothing we can do about that, just pointing it out.
How do we go about adding short forms to some of the flags?
I also want to try out the manpages and shell completion. How do we go about doing that?
@@ -346,6 +345,41 @@ EXPERIMENTAL: May be changed or removed. | |||
}(), | |||
}) | |||
|
|||
RegisterCommand(Command{ | |||
Name: "manpage", |
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.
This is cool, how do I actually use this on my system so I can do man caddy
and have it show me the manpages?
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.
My understanding is it would look something like this (not tested):
caddy manpage | gzip | sudo tee /usr/share/man/man1/caddy.1.gz && sudo mandb
But really, package installers will do this on install/update. I'll probably update the package install script for the apt repo after playing around with it for a while.
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.
Confirmed this isn't quite right, I assumed caddy manpage
would spit out to stdout but it takes a --directory
instead. But this is probably pretty close to being correct.
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.
This worked:
$ mkdir man
$ ./caddy manpage --directory man
$ gzip -r man/
$ sudo cp man/* /usr/share/man/man8/
$ sudo mandb
Will need to add this to the deb package installation later.
Woohoo! |
Caddy 2.6.0 does not support single-hyphen long options caddyserver/caddy#4565
Caddy 2.6.0 does not support single-hyphen long options caddyserver/caddy#4565
Caddy 2.6.0 does not support single-hyphen long options caddyserver/caddy#4565
This PR swaps the homegrown CLI parsing and management with github.com/muesli/coral, a fork of github.com/spf13/cobra. The switch brings the benefit of being able to generate shell-completions for major shells (PowerShell, bash, zsh, fish) and manpages. It's a POC/proposal just to see the impact of the change. It brings only 1 direct dependency and 3 transitive ones.
The current tricky part is figuring out how to adapt the current
caddycmd.Command
to*coral.Command
. The challenge is in mapping the flags. Once that is figured out, all is easy.P.S.: Never mind the deletions in commandfunc.go. I'm only using that to track which ones aren't ported yet. Once all is ported, I'll split the funcs back into the file.
Edit: As of Cobra v1.4.0, there is not dependency on Viper, which thins its dependency to minimum and matches coral, so swapping coral for cobra does not introduce any new deps (across the two).
Supercedes caddyserver/dist#73