Conversation
|
Neat, I think this is looking good so far. I don't know if maybe @mohammed90 wants to give some additional feedback or his thoughts when he has time. |
|
You reminded me of this lol 😅 I still need to work on some last details. I will post here again once that's done. |
armadi1809
force-pushed
the
migrate-xcaddy-to-cobra
branch
4 times, most recently
from
9940cd5 to
383f894
Compare
|
I think this is in a good spot for an initial review and perhaps some followup discussions. |
|
Sorry I lost track of this @armadi1809. Does this change the command line interface of xcaddy at all, or is it identical? |
|
@mholt no worries! It should be identical. It just adds in the help flag that is included with Cobra. |
mohammed90
left a comment
mohammed90
left a comment
There was a problem hiding this comment.
Thank you for tackling this! As with the others, I'll most likely do the review in rounds 😅 I like where this is going. Here's the first round of review.
armadi1809
force-pushed
the
migrate-xcaddy-to-cobra
branch
2 times, most recently
from
9ba7f4e to
218b084
Compare
|
Thanks for this first round of reviews Mohammed. All the issues should now be resolved. |
|
@armadi1809 Just wanted to say thanks for this effort. ive actually forked xcaddy for my own project to use the same build process (it is inspired by caddy's architecture) and this PR will also enable me to adopt the cli library. |
mohammed90
left a comment
mohammed90
left a comment
There was a problem hiding this comment.
We're getting closer, but there's a use-case that's broken now. It's possible to run xcaddy validate, xcaddy list-modules, or any caddy command -- which xcaddy takes and passes to caddy as-is. This is broken now. This is what I get:
/workspaces/xcaddy/caddy-git-fs (main) $ ../cmd/xcaddy/xcaddy list-modules
Error: unknown command "list-modules" for "xcaddy"
Run 'xcaddy --help' for usage.
unknown command "list-modules" for "xcaddy"
We need to figure it out because it's critical part of the Caddy module development.
cmd/cobra.go
Outdated
| Use: "xcaddy", | ||
| Long: "", |
There was a problem hiding this comment.
Please add content for the Short and Long fields
There was a problem hiding this comment.
I added short and long fields but still want your opinion on what should go/not go in these.
cmd/commands.go
Outdated
| Use: "version", | ||
| Long: "Getting xcaddy version", |
There was a problem hiding this comment.
Same here -- add Short and Long
There was a problem hiding this comment.
Same as other comment
armadi1809
force-pushed
the
migrate-xcaddy-to-cobra
branch
2 times, most recently
from
7064f00 to
73454d0
Compare
|
@mohammed90 The issue with the arguments to be passed to caddy should be resolved. I still want your opinion on the commands descriptions (short and long) though as I am not entirely sure about what exactly should go in these. Thanks. |
mohammed90
left a comment
mohammed90
left a comment
There was a problem hiding this comment.
I've amended the help lines, but I'm not very happy with my phrasings either. If @mholt and @francislavoie can take a look, that'd be great. Y'all have a good eye for this.
mholt
left a comment
mholt
left a comment
There was a problem hiding this comment.
tbh I think @mohammed90 's suggestions are great, and I would have done about the same.
Thanks for this change, it's looking good!
armadi1809
force-pushed
the
migrate-xcaddy-to-cobra
branch
from
73454d0 to
cf25878
Compare
armadi1809
force-pushed
the
migrate-xcaddy-to-cobra
branch
from
cf25878 to
66c26f9
Compare
mholt
left a comment
mholt
left a comment
There was a problem hiding this comment.
Maybe we can give this a shot. What do you think, @mohammed90 ?
mohammed90
left a comment
mohammed90
left a comment
There was a problem hiding this comment.
Thank you, @armadi1809! LGTM. Let's go!
|
Thanks for taking care of this @mohammed90 ! |
4 participants
This is still in draft mode (early stage) and not ready to be merged yet. Putting it here to get initial feedback (if possible) as I continue to work through this. I am also still finalizing the details about the proposed architecture.