-
-
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: fix regression in auto-detect of Caddyfile #6362
Conversation
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Can we add more test coverage for this? |
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.
Thanks for the quick patch. Some mostly unimportant thoughts...
wantErr: true, | ||
}, | ||
{ | ||
name: "json is not caddyfile but not error", |
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 case is still confusing to me 😅 and I almost wonder if it should be an error.
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.
We can duke it out later, but might be worth discussing at some point.
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.
I'm using an error return as a signal of ambiguity. We consider .json
a known extension, so it's not ambiguous. The determination is: .json is not ambiguous and isn't a Caddyfile.
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.
I use files like Caddyfile.test2
kinda thing, it's not uncommon to have like Caddyfile.dev
and Caddyfile.prod
. I'm not sure I understand how those will behave from looking at the code (just woke up and still away from home 😅)
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.
If you pass the --adpater
, it will go through as Caddyfile. If this is common, I'd rather we strip the whole .caddyfile
detection out.
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.
I think it is common, and historically it has gone with the "starts with Caddyfile" rule. I think we only want to say it's ambiguous if there is a loaded adapter that matches the extension, not otherwise. I think it's fine if Caddyfile.yaml
works if the yaml adapter isn't loaded. I realize that it's probably hard to test for though since we don't have any in Caddy itself. Maybe for testing, can make a dummy adapter?
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.
Then I think we should accept that users make silly mistakes such as passing --config Caddyfile.yaml
without --adapter
, or will do caddy run
without specifying the --adapter
which will cause the yaml file to be read as Caddyfile. It'll just fail silently (or loudly 🤷🏼♂️ ).
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.
Thank you!
I will merge this and tag a new release now.
Regression caused by #6356
Reported here:
Error: ambiguous config file format; please specify adapter (use --adapter)
since 2.8.2 #6363