cli: `fmt -check` should return early on diff (#16174)
The `nomad fmt -check` command incorrectly writes to file because we didn't return before writing the file on a diff. Fix this bug and update the command internals to differentiate between the write-to-file and write-to-stdout code paths, which are activated by different combinations of options and flags. The docstring for the `-list` and `-write` flags is also unclear and can be easily misread to be the opposite of the actual behavior. Clarify this and fix up the docs to match. This changeset also refactors the tests quite a bit so as to make the test outputs clear when something is incorrect.
This commit is contained in:
Tim Gross
GitHub
parent
1bbabdea37
commit
4fabad7f61
|
|
@ -0,0 +1,3 @@
|
|||
```release-note:bug
|
||||
cli: Fixed a bug where `nomad fmt -check` would overwrite the file being checked
|
||||
```
|
||||
|
|
@ -37,7 +37,8 @@ type FormatCommand struct {
|
|||
check bool
|
||||
checkSuccess bool
|
||||
recursive bool
|
||||
write bool
|
||||
writeFile bool
|
||||
writeStdout bool
|
||||
paths []string
|
||||
|
||||
stdin io.Reader
|
||||
|
|
@ -47,27 +48,31 @@ func (*FormatCommand) Help() string {
|
|||
helpText := `
|
||||
Usage: nomad fmt [flags] paths ...
|
||||
|
||||
Formats Nomad agent configuration and job file to a canonical format.
|
||||
If a path is a directory, it will recursively format all files
|
||||
with .nomad and .hcl extensions in the directory.
|
||||
|
||||
If you provide a single dash (-) as argument, fmt will read from standard
|
||||
input (STDIN) and output the processed output to standard output (STDOUT).
|
||||
Formats Nomad agent configuration and job file to a canonical format.
|
||||
If a path is a directory, it will recursively format all files
|
||||
with .nomad and .hcl extensions in the directory.
|
||||
|
||||
If you provide a single dash (-) as argument, fmt will read from standard
|
||||
input (STDIN) and output the processed output to standard output (STDOUT).
|
||||
|
||||
Format Options:
|
||||
|
||||
-list=false
|
||||
Don't list the files, which contain formatting inconsistencies.
|
||||
-check
|
||||
Check if the files are valid HCL files. If not, exit status of the
|
||||
command will be 1 and the incorrect files will not be formatted. This
|
||||
flag overrides any -write flag value.
|
||||
|
||||
-check
|
||||
Check if the files are valid HCL files. If not, exit status of the command
|
||||
will be 1 and the incorrect files will not be formatted.
|
||||
-list
|
||||
List the files which contain formatting inconsistencies. Defaults
|
||||
to -list=true.
|
||||
|
||||
-write=false
|
||||
Don't overwrite the input files.
|
||||
-recursive
|
||||
Process files in subdirectories. By default only the given (or current)
|
||||
directory is processed.
|
||||
|
||||
-recursive
|
||||
Process also files in subdirectories. By default only the given (or current) directory is processed.
|
||||
-write
|
||||
Overwrite the input files. Defaults to -write=true. Ignored if the input
|
||||
comes from STDIN.
|
||||
`
|
||||
|
||||
return strings.TrimSpace(helpText)
|
||||
|
|
@ -100,7 +105,7 @@ func (f *FormatCommand) Run(args []string) int {
|
|||
flags := f.Meta.FlagSet(f.Name(), FlagSetClient)
|
||||
flags.Usage = func() { f.Ui.Output(f.Help()) }
|
||||
flags.BoolVar(&f.check, "check", false, "")
|
||||
flags.BoolVar(&f.write, "write", true, "")
|
||||
flags.BoolVar(&f.writeFile, "write", true, "")
|
||||
flags.BoolVar(&f.list, "list", true, "")
|
||||
flags.BoolVar(&f.recursive, "recursive", false, "")
|
||||
|
||||
|
|
@ -121,11 +126,18 @@ func (f *FormatCommand) Run(args []string) int {
|
|||
if len(flags.Args()) == 0 {
|
||||
f.paths = []string{"."}
|
||||
} else if flags.Args()[0] == stdinArg {
|
||||
f.write = false
|
||||
f.writeStdout = true
|
||||
f.writeFile = false
|
||||
f.list = false
|
||||
} else {
|
||||
f.paths = flags.Args()
|
||||
}
|
||||
if f.check {
|
||||
f.writeFile = false
|
||||
}
|
||||
if !f.list && !f.writeFile {
|
||||
f.writeStdout = true
|
||||
}
|
||||
|
||||
f.fmt()
|
||||
|
||||
|
|
@ -251,19 +263,20 @@ func (f *FormatCommand) processFile(path string, r io.Reader) {
|
|||
f.Ui.Output(path)
|
||||
}
|
||||
|
||||
if f.write {
|
||||
if f.check {
|
||||
f.checkSuccess = false
|
||||
}
|
||||
|
||||
if f.writeFile {
|
||||
if err := os.WriteFile(path, out, 0644); err != nil {
|
||||
f.appendError(fmt.Errorf("Failed to write file %s: %w", path, err))
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
if f.check {
|
||||
f.checkSuccess = false
|
||||
}
|
||||
}
|
||||
|
||||
if !f.list && !f.write {
|
||||
if f.writeStdout {
|
||||
f.Ui.Output(string(out))
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ import (
|
|||
|
||||
"github.com/hashicorp/nomad/ci"
|
||||
"github.com/mitchellh/cli"
|
||||
"github.com/shoenig/test/must"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
|
@ -18,48 +19,80 @@ func TestFmtCommand(t *testing.T) {
|
|||
|
||||
const inSuffix = ".in.hcl"
|
||||
const expectedSuffix = ".out.hcl"
|
||||
tests := [][]string{
|
||||
{"nomad", "-check"},
|
||||
{"nomad", ""},
|
||||
{"job", "-check"},
|
||||
{"job", ""},
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
testFile string
|
||||
flags []string
|
||||
expectWrite bool
|
||||
expectCode int
|
||||
}{
|
||||
{
|
||||
name: "config with check",
|
||||
testFile: "nomad",
|
||||
flags: []string{"-check"},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "config without check",
|
||||
testFile: "nomad",
|
||||
flags: []string{},
|
||||
expectWrite: true,
|
||||
expectCode: 0,
|
||||
},
|
||||
{
|
||||
name: "job with check",
|
||||
testFile: "job",
|
||||
flags: []string{"-check"},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "job without check",
|
||||
testFile: "job",
|
||||
flags: []string{},
|
||||
expectWrite: true,
|
||||
expectCode: 0,
|
||||
},
|
||||
}
|
||||
tmpDir := t.TempDir()
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test[0]+test[1], func(t *testing.T) {
|
||||
inFile := filepath.Join("testdata", "fmt", test[0]+inSuffix)
|
||||
expectedFile := filepath.Join("testdata", "fmt", test[0]+expectedSuffix)
|
||||
fmtFile := filepath.Join(tmpDir, test[0]+".hcl")
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
tc := tc
|
||||
ci.Parallel(t)
|
||||
|
||||
input, err := os.ReadFile(inFile)
|
||||
require.NoError(t, err)
|
||||
tmpDir := t.TempDir()
|
||||
inFile := filepath.Join("testdata", "fmt", tc.testFile+inSuffix)
|
||||
expectedFile := filepath.Join("testdata", "fmt", tc.testFile+expectedSuffix)
|
||||
fmtFile := filepath.Join(tmpDir, tc.testFile+".hcl")
|
||||
|
||||
expected, err := os.ReadFile(expectedFile)
|
||||
require.NoError(t, err)
|
||||
must.NoError(t, err)
|
||||
|
||||
require.NoError(t, os.WriteFile(fmtFile, input, 0644))
|
||||
// copy the input file to the test tempdir so that we don't
|
||||
// overwrite the test input in source control
|
||||
input, err := os.ReadFile(inFile)
|
||||
must.NoError(t, err)
|
||||
must.NoError(t, os.WriteFile(fmtFile, input, 0644))
|
||||
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
}
|
||||
|
||||
var code int
|
||||
var expectedCode int
|
||||
if test[1] == "-check" {
|
||||
code = cmd.Run([]string{test[1], fmtFile})
|
||||
expectedCode = 1
|
||||
} else {
|
||||
code = cmd.Run([]string{fmtFile})
|
||||
expectedCode = 0
|
||||
}
|
||||
assert.Equal(t, expectedCode, code)
|
||||
flags := append(tc.flags, fmtFile)
|
||||
|
||||
code := cmd.Run(flags)
|
||||
must.Eq(t, tc.expectCode, code)
|
||||
|
||||
// compare the maybe-overwritten file contents
|
||||
actual, err := os.ReadFile(fmtFile)
|
||||
require.NoError(t, err)
|
||||
must.NoError(t, err)
|
||||
|
||||
assert.Equal(t, string(expected), string(actual))
|
||||
if tc.expectWrite {
|
||||
must.Eq(t, string(expected), string(actual))
|
||||
} else {
|
||||
must.Eq(t, string(input), string(actual))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -67,31 +100,36 @@ func TestFmtCommand(t *testing.T) {
|
|||
func TestFmtCommand_FromStdin(t *testing.T) {
|
||||
ci.Parallel(t)
|
||||
|
||||
stdinFake := bytes.NewBuffer(fmtFixture.input)
|
||||
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
stdin: stdinFake,
|
||||
tests := []struct {
|
||||
name string
|
||||
flags []string
|
||||
expectCode int
|
||||
}{
|
||||
{
|
||||
name: "with check",
|
||||
flags: []string{"-check", "-"},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "without check",
|
||||
flags: []string{"-"},
|
||||
expectCode: 0,
|
||||
},
|
||||
}
|
||||
|
||||
tests := []string{"-check", ""}
|
||||
for _, checkFlag := range tests {
|
||||
t.Run(checkFlag, func(t *testing.T) {
|
||||
var code int
|
||||
var expectedCode int
|
||||
|
||||
if checkFlag == "-check" {
|
||||
code = cmd.Run([]string{checkFlag, "-"})
|
||||
expectedCode = 1
|
||||
} else {
|
||||
code = cmd.Run([]string{"-"})
|
||||
expectedCode = 0
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
|
||||
stdinFake := bytes.NewBuffer(fmtFixture.input)
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
stdin: stdinFake,
|
||||
}
|
||||
|
||||
assert.Equal(t, expectedCode, code)
|
||||
assert.Contains(t, ui.OutputWriter.String(), string(fmtFixture.golden))
|
||||
code := cmd.Run(tc.flags)
|
||||
must.Eq(t, tc.expectCode, code)
|
||||
must.StrContains(t, string(fmtFixture.golden), ui.OutputWriter.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -106,28 +144,30 @@ func TestFmtCommand_FromWorkingDirectory(t *testing.T) {
|
|||
require.NoError(t, err)
|
||||
defer os.Chdir(cwd)
|
||||
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
tests := []struct {
|
||||
name string
|
||||
flags []string
|
||||
expectCode int
|
||||
}{
|
||||
{
|
||||
name: "with check",
|
||||
flags: []string{"-check"},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "without check",
|
||||
flags: []string{},
|
||||
expectCode: 0,
|
||||
},
|
||||
}
|
||||
|
||||
tests := []string{"-check", ""}
|
||||
for _, checkFlag := range tests {
|
||||
t.Run(checkFlag, func(t *testing.T) {
|
||||
var code int
|
||||
var expectedCode int
|
||||
|
||||
if checkFlag == "-check" {
|
||||
code = cmd.Run([]string{checkFlag})
|
||||
expectedCode = 1
|
||||
} else {
|
||||
code = cmd.Run([]string{})
|
||||
expectedCode = 0
|
||||
|
||||
}
|
||||
|
||||
assert.Equal(t, expectedCode, code)
|
||||
assert.Equal(t, fmt.Sprintf("%s\n", fmtFixture.filename), ui.OutputWriter.String())
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{Meta: Meta{Ui: ui}}
|
||||
code := cmd.Run(tc.flags)
|
||||
must.Eq(t, tc.expectCode, code)
|
||||
must.Eq(t, fmt.Sprintf("%s\n", fmtFixture.filename), ui.OutputWriter.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -135,57 +175,66 @@ func TestFmtCommand_FromWorkingDirectory(t *testing.T) {
|
|||
func TestFmtCommand_FromDirectoryArgument(t *testing.T) {
|
||||
tmpDir := fmtFixtureWriteDir(t)
|
||||
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
tests := []struct {
|
||||
name string
|
||||
flags []string
|
||||
expectCode int
|
||||
}{
|
||||
{
|
||||
name: "with check",
|
||||
flags: []string{"-check", tmpDir},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "without check",
|
||||
flags: []string{tmpDir},
|
||||
expectCode: 0,
|
||||
},
|
||||
}
|
||||
|
||||
tests := []string{"-check", ""}
|
||||
for _, checkFlag := range tests {
|
||||
t.Run(checkFlag, func(t *testing.T) {
|
||||
var code int
|
||||
var expectedCode int
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{Meta: Meta{Ui: ui}}
|
||||
|
||||
if checkFlag == "-check" {
|
||||
code = cmd.Run([]string{checkFlag, tmpDir})
|
||||
expectedCode = 1
|
||||
} else {
|
||||
code = cmd.Run([]string{tmpDir})
|
||||
expectedCode = 0
|
||||
|
||||
}
|
||||
|
||||
assert.Equal(t, expectedCode, code)
|
||||
assert.Equal(t, fmt.Sprintf("%s\n", filepath.Join(tmpDir, fmtFixture.filename)), ui.OutputWriter.String())
|
||||
code := cmd.Run(tc.flags)
|
||||
must.Eq(t, tc.expectCode, code)
|
||||
must.Eq(t,
|
||||
fmt.Sprintf("%s\n", filepath.Join(tmpDir, fmtFixture.filename)),
|
||||
ui.OutputWriter.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestFmtCommand_FromFileArgument(t *testing.T) {
|
||||
tmpDir := fmtFixtureWriteDir(t)
|
||||
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{
|
||||
Meta: Meta{Ui: ui},
|
||||
}
|
||||
path := filepath.Join(tmpDir, fmtFixture.filename)
|
||||
|
||||
tests := []string{"-check", ""}
|
||||
for _, checkFlag := range tests {
|
||||
t.Run(checkFlag, func(t *testing.T) {
|
||||
var code int
|
||||
var expectedCode int
|
||||
tests := []struct {
|
||||
name string
|
||||
flags []string
|
||||
expectCode int
|
||||
}{
|
||||
{
|
||||
name: "with check",
|
||||
flags: []string{"-check", path},
|
||||
expectCode: 1,
|
||||
},
|
||||
{
|
||||
name: "without check",
|
||||
flags: []string{path},
|
||||
expectCode: 0,
|
||||
},
|
||||
}
|
||||
|
||||
if checkFlag == "-check" {
|
||||
code = cmd.Run([]string{checkFlag, path})
|
||||
expectedCode = 1
|
||||
} else {
|
||||
code = cmd.Run([]string{path})
|
||||
expectedCode = 0
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
ui := cli.NewMockUi()
|
||||
cmd := &FormatCommand{Meta: Meta{Ui: ui}}
|
||||
|
||||
}
|
||||
assert.Equal(t, expectedCode, code)
|
||||
assert.Equal(t, fmt.Sprintf("%s\n", path), ui.OutputWriter.String())
|
||||
code := cmd.Run(tc.flags)
|
||||
must.Eq(t, tc.expectCode, code)
|
||||
must.Eq(t, fmt.Sprintf("%s\n", path), ui.OutputWriter.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -232,6 +281,6 @@ var fmtFixture = struct {
|
|||
golden []byte
|
||||
}{
|
||||
filename: "nomad.hcl",
|
||||
input: []byte(`client {enabled = true}`),
|
||||
golden: []byte(`client { enabled = true }`),
|
||||
input: []byte("client {enabled = true}"),
|
||||
golden: []byte("client { enabled = true }\n\n"),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,16 +26,19 @@ If you provide a single dash (-) as argument, fmt will read from standard input
|
|||
|
||||
## Format Options:
|
||||
|
||||
- `-list=false` : Don't list the files, which contain formatting inconsistencies.
|
||||
- `-check` : Check if the files are valid HCL files. If not, exit status of the command
|
||||
will be 1 and the incorrect files will not be formatted.
|
||||
- `-write=false` : Don't overwrite the input files.
|
||||
- `-recursive` : Process also files in subdirectories. By default only the given (or current) directory is processed.
|
||||
- `-check`: Check if the files are valid HCL files. If not, exit status of the
|
||||
command will be 1 and the incorrect files will not be formatted. This flag
|
||||
overrides any `-write` flag value.
|
||||
- `-list`: List the files which contain formatting inconsistencies. Defaults to
|
||||
`-list=true`.
|
||||
- `-recursive`: Process files in subdirectories. By default only the given (or
|
||||
current) directory is processed.
|
||||
- `-write`: Overwrite the input files. Defaults to `-write=true`.
|
||||
|
||||
## Examples
|
||||
|
||||
```shell-session
|
||||
$ cat agent.hcl
|
||||
$ cat agent.hcl
|
||||
server {
|
||||
enabled = true
|
||||
bootstrap_expect = 1
|
||||
|
|
@ -44,11 +47,15 @@ server {
|
|||
client {
|
||||
enabled = true
|
||||
}
|
||||
```
|
||||
|
||||
```shell-session
|
||||
$ nomad fmt
|
||||
|
||||
agent.hcl
|
||||
$ cat agent.hcl
|
||||
```
|
||||
|
||||
```shell-session
|
||||
$ cat agent.hcl
|
||||
server {
|
||||
enabled = true
|
||||
bootstrap_expect = 1
|
||||
|
|
|
|||
Loading…
Reference in New Issue