Repository: caddyserver/caddy (language: go)

Issue:
Allow returning 1 if lint fails
Right now we have

```shell
Usage:
  caddy fmt [flags]

Flags:
      --diff        Print the differences between the input file and the formatted output
  -h, --help        help for fmt
      --overwrite   Overwrite the input file with the results
```

I originally thought `caddy fmt` will return `1` if failed like most other linters. It turns out not. It returns formatted content instead.
If we don't want to change the behavior of `caddy fmt`, it would be great to provide another flag to allow returning `1` for failure. Thanks! 😃


Discussion / hints:
Ah yeah, that's a reasonable change.

PRs welcome! Should be very simple.
@Hongbo-Miao @francislavoie 
Hi, I would like to work on this issue.
How is that like this?
```
% ./caddy fmt        
{
        debug
        log stdout
        admin 0.0.0.0:2018
        grace_period 1s
}

:8888 {
        log
        root * /home/u5surf/test
        php_fastcgi unix//run/php/php8.1-fpm.sock {
                #buffer_requests
                #flush_interval -1
        }
}
% echo $?
0
% ./caddy fmt --exit1
{
        debug
        log stdout
        admin 0.0.0.0:2018
        grace_period 1s
}

:8888 {
        log
        root * /home/u5surf/test
        php_fastcgi unix//run/php/php8.1-fpm.sock {
                #buffer_requests
                #flush_interval -1
        }
}
Error: diff found
% echo $?            
1
% diff Caddyfile <(./caddy fmt)
8c8
< :8888  {
---
> :8888 {
```
Thanks @u5surf ! Really appreciate! 🚀
Personally I would vote for a breaking change to change the existing behavior of `caddy fmt` as it aligns with all other linters.

We can still show formatted result, but return `1` if failed. Although it is a breaking change, however, I don't think anyone is relying on the result that always `0` before in any pipeline as it is not useful. So maybe not really count a breaking change.

Let's wait @francislavoie for more thoughts! 😃
I agree, I highly doubt anyone was relying on the existing return code. Let's just return 1 by default if the formatted output has any differences from the original.
@Hongbo-Miao @francislavoie 
OK, I agree with your suggestions.
I consider there is only one problem, `caddyCommandToCoral`  ignores the exit code( the first return value) which the command function returns.
Thus if we’d like to be exit 1 in this scenario, it must returns something error messages with `fmt.Errorf` in `runFmt` in case of current `caddyCommandToCoral`.

https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/cobra.go#L114-L115
/cc @mohammed90 since you have a better understanding of the cobra stuff (we should probably rename that function from `Coral` -> `Cobra` :sweat_smile:)
> @Hongbo-Miao @francislavoie 
> OK, I agree with your suggestions.
> I consider there is only one problem, `caddyCommandToCoral`  ignores the exit code( the first return value) which the command function returns.
> Thus if we’d like to be exit 1 in this scenario, it must returns something error messages with `fmt.Errorf` in `runFmt` in case of current `caddyCommandToCoral`.
> 
> https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/cobra.go#L114-L115

You should look into the `cmdFmt` function, not `cmdToCoral`.

https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/commandfuncs.go#L532

It should return an error from there if the input and output mismatch. You'll face a tricky part where we're already returning `1` if we fail to read the input. Should the code for both be the same? Perhaps.

@francislavoie, do you think an extra flag is necessary? I doubt it's breaking anyone if we switch from returning 0 to returning 1.
No I don't think we need a flag at all, as I said above.
@mohammed90 
> You should look into the cmdFmt function, not cmdToCoral.

`runFmt` is a member of `Command` struct, which is the argument of the `RegisterCommand` function.
https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/commands.go#L304-L308

We can see the `cmdToCoral` function is called at last of `RegisterCommand` function. which received the cmd is the entity　of `Command`.
https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/commands.go#L491-L507
That is why I focus on the `cmdToCoral`, and I can see the entity of the `runFmt` is called in `cmdToCoral` but the first return value which stands for exit code is ignored in cobra.go L.114

https://github.com/caddyserver/caddy/blob/223cbe3d0b50487117c785f0755bb80a9ee65010/cmd/cobra.go#L107-L117

I'd like to say is `RunE` writes some  error messages if the function returns error which is stands for the second return value in cobra.go L.114. Thus we should set the error messages if the input and output mismatch. 

Once you can admit bellow output which includes some error messages like `Error: diff found`, I would make a PR easily now.
```
% ./caddy fmt
{
        debug
        log stdout
        admin 0.0.0.0:2018
        grace_period 1s
}

:8888 {
        log
        root * /home/u5surf/test
        php_fastcgi unix//run/php/php8.1-fpm.sock {
                #buffer_requests
                #flush_interval -1
        }
}
Error: diff found
% echo $?            
1
```

Alternative: 
If you would like to ignore such messages it can set SilenceErrors is true.
But I consider that it will be a breaking change seriously because it forces to ignore the all of the error messages which are made by its subcommand function.

https://github.com/spf13/cobra/blob/main/command.go#L1050
Returning an error implies an exit code 1, see:
https://github.com/caddyserver/caddy/blob/master/cmd/main.go#L65

For the sake of this ticket, returning an error should be sufficient.

Wiring the other exit codes requires more involved work. Currently in my head, I envision a custom error type that returns/embeds the exit code to be used. However, those other exit codes (defined in sigtrap.go) need to be traced first to validate if they're truly returned from one of the `cmd*` funcs. If not, none of this paragraph is necessary.

Fix patch:
diff --git a/caddyconfig/caddyfile/adapter.go b/caddyconfig/caddyfile/adapter.go
index d5bb7c22183..b924325b076 100644
--- a/caddyconfig/caddyfile/adapter.go
+++ b/caddyconfig/caddyfile/adapter.go
@@ -54,7 +54,7 @@ func (a Adapter) Adapt(body []byte, options map[string]any) ([]byte, []caddyconf
 
 	// lint check: see if input was properly formatted; sometimes messy files files parse
 	// successfully but result in logical errors (the Caddyfile is a bad format, I'm sorry)
-	if warning, different := formattingDifference(filename, body); different {
+	if warning, different := FormattingDifference(filename, body); different {
 		warnings = append(warnings, warning)
 	}
 
@@ -63,10 +63,10 @@ func (a Adapter) Adapt(body []byte, options map[string]any) ([]byte, []caddyconf
 	return result, warnings, err
 }
 
-// formattingDifference returns a warning and true if the formatted version
+// FormattingDifference returns a warning and true if the formatted version
 // is any different from the input; empty warning and false otherwise.
 // TODO: also perform this check on imported files
-func formattingDifference(filename string, body []byte) (caddyconfig.Warning, bool) {
+func FormattingDifference(filename string, body []byte) (caddyconfig.Warning, bool) {
 	// replace windows-style newlines to normalize comparison
 	normalizedBody := bytes.Replace(body, []byte("\r\n"), []byte("\n"), -1)
 
diff --git a/cmd/commandfuncs.go b/cmd/commandfuncs.go
index e3daae59a8f..bc4c22781a0 100644
--- a/cmd/commandfuncs.go
+++ b/cmd/commandfuncs.go
@@ -576,6 +576,10 @@ func cmdFmt(fl Flags) (int, error) {
 		fmt.Print(string(output))
 	}
 
+	if warning, diff := caddyfile.FormattingDifference(formatCmdConfigFile, input); diff {
+		return caddy.ExitCodeFailedStartup, fmt.Errorf("%s:%d: Caddyfile input is not formatted", warning.File, warning.Line)
+	}
+
 	return caddy.ExitCodeSuccess, nil
 }
 


Test patch: