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

caddy fmt will silently ignore all but the first file path passed to it #6702

Open
alexwlchan opened this issue Nov 22, 2024 · 4 comments
Open
Labels
bug 🐞 Something isn't working good first issue 🐤 Good for newcomers

Comments

@alexwlchan
Copy link

If you call caddy fmt with the path to more than one file, it only formats the first file.

For example:

$ caddy fmt 1.Caddyfile 2.Caddyfile --overwrite

then 1.Caddyfile will be reformatted, but 2.Caddyfile will be silently ignored.

If I look in the docs, I can see that caddy fmt only supports passing a single path – but it took me a while to realise that! This is different from other formatting tools I've used (e.g. black, terraform fmt) where you can pass multiple files and format them all together.

I think it would be less confusing if caddy fmt either:

  • Formatted every <path> passed to it, or
  • Threw an error if you tried to pass more than one <path>

Context

I'd written a command to find all the Caddyfiles in my repo and format them:

$ find . -name Caddyfile | xargs --verbose caddy fmt
caddy fmt 1/Caddyfile 2/Caddyfile

and I was getting inconsistent results between different machines – it took me a while to notice that find was returning results in an inconsistent order, and caddy was only formatting the first in the list.

I don't really mind what caddy does, but the current behaviour caused a lot of confusion.

Version

$ caddy --version
v2.8.4 h1:q3pe0wpBj1OcHFZ3n/1nl4V4bxBrYoSoab7rL9BMYNk=
@mholt
Copy link
Member

mholt commented Nov 22, 2024

Thanks for the report. Should be an easy fix, I suppose -- anyone is welcome to contribute!

@alexwlchan
Copy link
Author

Noting in case I have time to pick this up: this looks like the relevant function.

caddy/cmd/commandfuncs.go

Lines 561 to 619 in eddbccd

func cmdFmt(fl Flags) (int, error) {
configFile := fl.Arg(0)
if configFile == "" {
configFile = "Caddyfile"
}
// as a special case, read from stdin if the file name is "-"
if configFile == "-" {
input, err := io.ReadAll(os.Stdin)
if err != nil {
return caddy.ExitCodeFailedStartup,
fmt.Errorf("reading stdin: %v", err)
}
fmt.Print(string(caddyfile.Format(input)))
return caddy.ExitCodeSuccess, nil
}
input, err := os.ReadFile(configFile)
if err != nil {
return caddy.ExitCodeFailedStartup,
fmt.Errorf("reading input file: %v", err)
}
output := caddyfile.Format(input)
if fl.Bool("overwrite") {
if err := os.WriteFile(configFile, output, 0o600); err != nil {
return caddy.ExitCodeFailedStartup, fmt.Errorf("overwriting formatted file: %v", err)
}
return caddy.ExitCodeSuccess, nil
}
if fl.Bool("diff") {
diff := difflib.Diff(
strings.Split(string(input), "\n"),
strings.Split(string(output), "\n"))
for _, d := range diff {
switch d.Delta {
case difflib.Common:
fmt.Printf(" %s\n", d.Payload)
case difflib.LeftOnly:
fmt.Printf("- %s\n", d.Payload)
case difflib.RightOnly:
fmt.Printf("+ %s\n", d.Payload)
}
}
} else {
fmt.Print(string(output))
}
if warning, diff := caddyfile.FormattingDifference(configFile, input); diff {
return caddy.ExitCodeFailedStartup, fmt.Errorf(`%s:%d: Caddyfile input is not formatted; Tip: use '--overwrite' to update your Caddyfile in-place instead of previewing it. Consult '--help' for more options`,
warning.File,
warning.Line,
)
}
return caddy.ExitCodeSuccess, nil
}

@francislavoie
Copy link
Member

Well, you can only have one root Caddyfile, so I don't think it really makes sense to try to format more than one. If you're doing that because you have import etc, then I think we should update fmt to follow imports, possibly, and also format those files (when -w is used).

So I think yeah disallowing more than one file probably makes sense.

Also while we're at it I think we could add --config as a flag (which is the same as the one arg, and if set args can be empty), cause most of Caddy's other commands use --config/-c to specify the config file, ran into that on occasion as a footgun when I'm hitting UP in my terminal to edit the command and run another.

@alexwlchan
Copy link
Author

If you're doing that because you have import etc, then I think we should update fmt to follow imports, possibly, and also format those files (when -w is used).

Yeah, imported files is precisely where I ran into this.

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

No branches or pull requests

3 participants