From 01d5b42cf7a6b3c020d8c27b0d60eca142838cfd Mon Sep 17 00:00:00 2001 From: Jean-Hadrien Chabran Date: Wed, 3 Jul 2024 16:14:38 +0200 Subject: [PATCH] chore(local): make sg handle empty secret file gracefully (#63614) Previously, `sg` would trip on an empty `sg.secrets.json`. It now treats it the same way as if the file wasn't there at all. Improved usage text as I was there already. ## Test plan CI + added a unit test + local test. ## Changelog --- dev/sg/internal/secrets/store.go | 10 ++++- dev/sg/internal/secrets/store_test.go | 56 +++++++++++++++++++++++---- dev/sg/sg_secret.go | 10 ++--- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/dev/sg/internal/secrets/store.go b/dev/sg/internal/secrets/store.go index 589060e39bc..270c2d877b6 100644 --- a/dev/sg/internal/secrets/store.go +++ b/dev/sg/internal/secrets/store.go @@ -75,7 +75,15 @@ func LoadFromFile(filepath string) (*Store, error) { } defer f.Close() dec := json.NewDecoder(f) - return s, dec.Decode(&s.m) + if err := dec.Decode(&s.m); err != nil { + // Ignore EOF which is returned when the file is empty, we just pretend the file isn't there. + // Note that invalid JSON might still return "unexpected EOF" and + // we let that one get through. + if !errors.Is(err, io.EOF) { + return nil, err + } + } + return s, nil } // Write serializes the store content in the given writer. diff --git a/dev/sg/internal/secrets/store_test.go b/dev/sg/internal/secrets/store_test.go index 142331d2e2e..6252d33d4a5 100644 --- a/dev/sg/internal/secrets/store_test.go +++ b/dev/sg/internal/secrets/store_test.go @@ -18,24 +18,64 @@ func TestSecrets(t *testing.T) { store := newStore("") err := store.Put("foo", data) if err != nil { - t.Fatalf("want no error, got %v", err) + t.Fatalf("want no error, got %q", err) } want := data got := mySecrets{} err = store.Get("foo", &got) if err != nil { - t.Fatalf("%v", err) + t.Fatalf("want no error when getting secret, but got: %q", err) } if diff := cmp.Diff(want, got); diff != "" { t.Fatalf("wrong secret data. (-want +got):\n%s", diff) } }) + t.Run("LoadFile returns an error on invalid JSON", func(t *testing.T) { + f, err := os.CreateTemp(os.TempDir(), "secrets*.json") + if err != nil { + t.Fatalf("couldn't create temp secret file: %q", err) + } + if _, err := f.WriteString(`{"foo":1`); err != nil { + t.Fatalf("couldn't write in temp secret file: %q", err) + } + f.Close() + filepath := f.Name() + t.Cleanup(func() { + _ = os.Remove(filepath) + }) + + _, err = LoadFromFile(filepath) + if err == nil { + t.Fatal("want an error but got none") + } + }) + + t.Run("LoadFile doesn't fail when file is empty", func(t *testing.T) { + f, err := os.CreateTemp(os.TempDir(), "secrets*.json") + if err != nil { + t.Fatalf("couldn't create temp secret file: %q", err) + } + f.Close() + filepath := f.Name() + t.Cleanup(func() { + _ = os.Remove(filepath) + }) + + got, err := LoadFromFile(filepath) + if err != nil { + t.Fatalf("want no error when loading an empty file, but got %q instead", err) + } + if got == nil { + t.Fatal("want store to not be nil") + } + }) + t.Run("SaveFile and LoadFile", func(t *testing.T) { f, err := os.CreateTemp(os.TempDir(), "secrets*.json") if err != nil { - t.Fatalf("%v", err) + t.Fatalf("couldn't create temp secret file: %q", err) } f.Close() filepath := f.Name() @@ -47,19 +87,21 @@ func TestSecrets(t *testing.T) { // Assign a secret and save it s, err := LoadFromFile(filepath) if err != nil { - t.Fatalf("%v", err) + t.Fatalf("couldn't load temp secret file: %q", err) } data := map[string]any{"key": "val"} - s.Put("foo", data) + if err := s.Put("foo", data); err != nil { + t.Fatalf("want no error when putting secret, got %q", err) + } err = s.SaveFile() if err != nil { - t.Fatalf("%v", err) + t.Fatalf("failed to save secrets: %q", err) } // Fetch it back and compare got, err := LoadFromFile(filepath) if err != nil { - t.Fatalf("%v", err) + t.Fatalf("couldn't load temp secret file: %q", err) } if diff := cmp.Diff(s.m, got.m); diff != "" { t.Fatalf("(-want +got):\n%s", diff) diff --git a/dev/sg/sg_secret.go b/dev/sg/sg_secret.go index 85948c74a1e..c87cf616852 100644 --- a/dev/sg/sg_secret.go +++ b/dev/sg/sg_secret.go @@ -20,20 +20,18 @@ var ( secretCommand = &cli.Command{ Name: "secret", Usage: "Manipulate secrets stored in memory and in file", - UsageText: ` -# List all secrets stored in your local configuration. + UsageText: `# List all secrets stored in your local configuration. sg secret list -# Remove the secrets associated with buildkite (sg ci build) - supports autocompletion for -# ease of use +# Remove the secrets associated with buildkite (sg ci build) - (supports bash autocompletion). sg secret reset buildkite `, Category: category.Env, Subcommands: []*cli.Command{ { Name: "reset", - ArgsUsage: "<...key>", - Usage: "Remove a specific secret from secrets file", + ArgsUsage: "key1 key2 ...", + Usage: "Remove a individual secret(s) from secrets file (see 'list' for getting the keys)", Action: resetSecretExec, BashComplete: completions.CompleteArgs(bashCompleteSecrets), },