diff --git a/.changelog/16174.txt b/.changelog/16174.txt new file mode 100644 index 000000000..8c5db1574 --- /dev/null +++ b/.changelog/16174.txt @@ -0,0 +1,3 @@ +```release-note:bug +cli: Fixed a bug where `nomad fmt -check` would overwrite the file being checked +``` diff --git a/command/fmt.go b/command/fmt.go index 0156c295a..fe64c20e2 100644 --- a/command/fmt.go +++ b/command/fmt.go @@ -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)) } } diff --git a/command/fmt_test.go b/command/fmt_test.go index 22f4edc44..082656169 100644 --- a/command/fmt_test.go +++ b/command/fmt_test.go @@ -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"), } diff --git a/website/content/docs/commands/fmt.mdx b/website/content/docs/commands/fmt.mdx index 2788805d8..7cf9ace51 100644 --- a/website/content/docs/commands/fmt.mdx +++ b/website/content/docs/commands/fmt.mdx @@ -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