From 68abcb80a0c7a61cc1619e96bfee21978e2227e1 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 15 Feb 2023 14:06:31 -0500 Subject: [PATCH] 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. --- .changelog/16174.txt | 3 + command/fmt.go | 59 +++--- command/fmt_test.go | 263 +++++++++++++++----------- website/content/docs/commands/fmt.mdx | 23 ++- 4 files changed, 210 insertions(+), 138 deletions(-) create mode 100644 .changelog/16174.txt diff --git a/.changelog/16174.txt b/.changelog/16174.txt new file mode 100644 index 000000000000..8c5db1574953 --- /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 0156c295a3b8..fe64c20e20f7 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 22f4edc4476e..082656169745 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 - - if checkFlag == "-check" { - code = cmd.Run([]string{checkFlag, tmpDir}) - expectedCode = 1 - } else { - code = cmd.Run([]string{tmpDir}) - 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", 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 2788805d89a9..7cf9ace51c1c 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