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

cmd: fix auto-detetction of .caddyfile extension #6356

Merged
merged 4 commits into from
Jun 2, 2024
Merged

cmd: fix auto-detetction of .caddyfile extension #6356

merged 4 commits into from
Jun 2, 2024

Conversation

mohammed90
Copy link
Member

Fix #6355

@mohammed90 mohammed90 added the bug 🐞 Something isn't working label May 31, 2024
@mohammed90 mohammed90 added this to the v2.8.2 milestone May 31, 2024
@francislavoie
Copy link
Member

I think this existed to deal with #5919 (comment) i.e. Caddyfile.yaml if yaml is loaded as an adapter, should use that instead of the Caddyfile adapter.

@mohammed90
Copy link
Member Author

mohammed90 commented May 31, 2024

I think this existed to deal with #5919 (comment) i.e. Caddyfile.yaml if yaml is loaded as an adapter, should use that instead of the Caddyfile adapter.

They would have specified the --adapter flag. I've added a comment.

@mholt
Copy link
Member

mholt commented May 31, 2024

Are you sure though? The point of this is to presume when the Caddyfile is being used: Caddyfile.yaml is clearly ambiguous (and incorrect, since a yaml file isn't Caddyfile, and a Caddyfile isn't yaml) -- what happens if they don't use the --adapter flag? Do we presume Caddyfile or YAML?

@mohammed90
Copy link
Member Author

what happens if they don't use the --adapter flag? Do we presume Caddyfile or YAML?

I can add a condition for such pesky scenarios 🙂

if adapterName == "" && startsOrEndsInCaddyfile {
    return nil, "", fmt.Error("ambiguous... blablabbla")
}

@mholt
Copy link
Member

mholt commented May 31, 2024

Ok, yeah, that sounds better. Maybe an error is better than ignoring it like we were doing. (But yeah, it's still important that we don't proceed, either way.)

Signed-off-by: Mohammed Al Sahaf <[email protected]>
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mohammed!

@mholt mholt enabled auto-merge (squash) June 2, 2024 03:38
@mholt mholt merged commit 15faeac into master Jun 2, 2024
22 of 23 checks passed
@mholt mholt deleted the fix-6355 branch June 2, 2024 03:49
@graphixillusion
Copy link

With this new modification, in version 2.8.2 my Caddyfile isn't recognized anymore. I need to force it using -c Caddyfile --adapter caddyfile. If not i get ambigous config message error. Same Caddyfile works without problems in 2.8.1

@mohammed90
Copy link
Member Author

With this new modification, in version 2.8.2 my Caddyfile isn't recognized anymore. I need to force it using -c Caddyfile --adapter caddyfile. If not i get ambigous config message error. Same Caddyfile works without problems in 2.8.1

My bad. Let me work on the fix. I'm sorry.

@cattyhouse
Copy link

official website mentions Caddyfile as config name (note the cap C), but since this change 'Caddyfile' will produce error. You may need to modify the documents of your official website or add Caddyfile to this line startsOrEndsInCaddyfile := strings.HasPrefix(baseConfig, "caddyfile") || strings.HasSuffix(baseConfig, ".caddyfile")

@mohammed90
Copy link
Member Author

You may need to modify the documents of your official website

That's not necessary. It's a regression. Please see the linked PR.

@avluis
Copy link

avluis commented Jun 2, 2024

Aha! I knew it; appreciate the very quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using the file ext caddyfile for a config file does not work
6 participants