From 08fdea2b92cfc4a2aade5edcb294c03fd1d1eaee Mon Sep 17 00:00:00 2001 From: Simon Bein Date: Thu, 2 Nov 2023 19:44:12 +0100 Subject: [PATCH] refactor: reduce the dependency on the default config in tests --- cmd/cleanup.go | 20 ++++++----- cmd/cleanup_test.go | 83 ++++++++++++++++++++++++--------------------- cmd/delete.go | 5 +-- cmd/import.go | 3 +- cmd/set.go | 14 ++++---- cmd/set_test.go | 8 +++-- config/config.go | 8 ++--- go.mod | 2 +- konf/id_test.go | 17 ---------- konf/konfig.go | 1 - konf/split.go | 2 -- konf/split_test.go | 9 ++--- store/store.go | 7 ++-- store/store_test.go | 5 ++- utils/dir_test.go | 12 ++++++- 15 files changed, 97 insertions(+), 99 deletions(-) diff --git a/cmd/cleanup.go b/cmd/cleanup.go index b4b05cf..5c54eb1 100644 --- a/cmd/cleanup.go +++ b/cmd/cleanup.go @@ -10,6 +10,7 @@ import ( "github.com/simontheleg/konf-go/config" "github.com/simontheleg/konf-go/konf" log "github.com/simontheleg/konf-go/log" + "github.com/simontheleg/konf-go/store" "github.com/spf13/afero" "github.com/spf13/cobra" ) @@ -23,12 +24,14 @@ An active config is considered unused when no process points to it anymore`, RunE: func(cmd *cobra.Command, args []string) error { fs := afero.NewOsFs() - err := cleanLeftOvers(fs) + sm := &store.Storemanager{Activedir: config.ActiveDir(), Storedir: config.StoreDir(), Fs: fs} + + err := cleanLeftOvers(sm) if err != nil { return err } - err = selfClean(fs) + err = selfClean(sm) if err != nil { return err } @@ -41,11 +44,12 @@ An active config is considered unused when no process points to it anymore`, // it is required as the idempotent clean would delete all files that // do not belong to any process anymore, but of course the current process // is still running at this time -func selfClean(f afero.Fs) error { +func selfClean(sm *store.Storemanager) error { pid := os.Getppid() - fpath := konf.IDFromProcessID(pid).ActivePath() - err := f.Remove(fpath) + konfID := konf.IDFromProcessID(pid) + fpath := sm.ActivePathFromID(konfID) + err := sm.Fs.Remove(fpath) if errors.Is(err, fs.ErrNotExist) { log.Info("current konf '%s' was already deleted, nothing to self-cleanup\n", fpath) @@ -64,8 +68,8 @@ func selfClean(f afero.Fs) error { // any leftovers that can occur if a previous session was not cleaned up nicely. This is // necessary as we cannot tell a user that a selfClean has failed if they close the shell // session before -func cleanLeftOvers(f afero.Fs) error { - konfs, err := afero.ReadDir(f, config.ActiveDir()) +func cleanLeftOvers(sm *store.Storemanager) error { + konfs, err := afero.ReadDir(sm.Fs, sm.Activedir) if err != nil { return err @@ -86,7 +90,7 @@ func cleanLeftOvers(f afero.Fs) error { } if p == nil { - err := f.Remove(konfID.ActivePath()) + err := sm.Fs.Remove(sm.ActivePathFromID(konfID)) if err != nil { return err } diff --git a/cmd/cleanup_test.go b/cmd/cleanup_test.go index 4998f16..023d860 100644 --- a/cmd/cleanup_test.go +++ b/cmd/cleanup_test.go @@ -9,15 +9,16 @@ import ( "syscall" "testing" - "github.com/simontheleg/konf-go/config" "github.com/simontheleg/konf-go/konf" + "github.com/simontheleg/konf-go/store" "github.com/simontheleg/konf-go/testhelper" "github.com/simontheleg/konf-go/utils" "github.com/spf13/afero" ) func TestSelfClean(t *testing.T) { - activeDir := config.ActiveDir() + activeDir := "./konf/active" + storeDir := "./konf/store" ppid := os.Getppid() @@ -28,26 +29,28 @@ func TestSelfClean(t *testing.T) { NotExpFiles []string }{ "PID FS": { - ppidFS(), + ppidFS(activeDir), nil, - []string{activeDir + "/abc", konf.KonfID("1234").ActivePath()}, + []string{activeDir + "/abc", activeDir + "/1234"}, []string{activeDir + "/" + fmt.Sprint(ppid) + ".yaml"}, }, "PID file deleted by external source": { - ppidFileMissing(), + ppidFileMissing(activeDir), nil, - []string{activeDir + "/abc", konf.KonfID("1234").ActivePath()}, + []string{activeDir + "/abc", activeDir + "/1234"}, []string{}, }, - // Unfortunately it was not possible with afero to test what happens if - // someone changes the active dir permissions an we cannot delete it anymore - // apparently in the memFS afero can just delete these files + // Unfortunately it was not possible with afero memFS to test what happens if + // someone changes the active dir permissions and we cannot delete it anymore. + // Apparently in the memFS afero can just delete these files, regardless of + // permissions :D } for name, tc := range tt { t.Run(name, func(t *testing.T) { - err := selfClean(tc.Fs) + sm := &store.Storemanager{Fs: tc.Fs, Activedir: activeDir, Storedir: storeDir} + err := selfClean(sm) if !testhelper.EqualError(err, tc.ExpError) { t.Errorf("Want error '%s', got '%s'", tc.ExpError, err) @@ -74,26 +77,26 @@ func TestSelfClean(t *testing.T) { } } -func ppidFS() afero.Fs { +func ppidFS(activeDir string) afero.Fs { ppid := os.Getppid() - fs := ppidFileMissing() + fs := ppidFileMissing(activeDir) sm := testhelper.SampleKonfManager{} - afero.WriteFile(fs, konf.IDFromProcessID(ppid).ActivePath(), []byte(sm.SingleClusterSingleContextEU()), utils.KonfPerm) + afero.WriteFile(fs, activeDir+"/"+fmt.Sprint(ppid), []byte(sm.SingleClusterSingleContextEU()), utils.KonfPerm) return fs } -func ppidFileMissing() afero.Fs { +func ppidFileMissing(activeDir string) afero.Fs { fs := afero.NewMemMapFs() sm := testhelper.SampleKonfManager{} - afero.WriteFile(fs, config.ActiveDir()+"/abc", []byte("I am not even a kubeconfig, what am I doing here?"), utils.KonfPerm) - afero.WriteFile(fs, konf.KonfID("1234").ActivePath(), []byte(sm.SingleClusterSingleContextEU()), utils.KonfPerm) + afero.WriteFile(fs, activeDir+"/abc", []byte("I am not even a kubeconfig, what am I doing here?"), utils.KonfPerm) + afero.WriteFile(fs, activeDir+"/1234", []byte(sm.SingleClusterSingleContextEU()), utils.KonfPerm) return fs } func TestCleanLeftOvers(t *testing.T) { tt := map[string]struct { - Setup func(t *testing.T) (afero.Fs, []*exec.Cmd, []*exec.Cmd) + Setup func(t *testing.T, sm *store.Storemanager) ([]*exec.Cmd, []*exec.Cmd) ExpErr error }{ "all procs still running": { @@ -116,21 +119,23 @@ func TestCleanLeftOvers(t *testing.T) { for name, tc := range tt { t.Run(name, func(t *testing.T) { - f, cmdsRunning, cmdsStopped := tc.Setup(t) + sm := &store.Storemanager{Fs: afero.NewMemMapFs(), Activedir: "./konf/active", Storedir: "./konf/store"} + cmdsRunning, cmdsStopped := tc.Setup(t, sm) t.Cleanup(func() { cleanUpRunningCmds(t, cmdsRunning) }) - err := cleanLeftOvers(f) + err := cleanLeftOvers(sm) if !errors.Is(err, tc.ExpErr) { t.Errorf("Want error '%s', got '%s'", tc.ExpErr, err) } for _, cmd := range cmdsRunning { - fpath := konf.IDFromProcessID(cmd.Process.Pid).ActivePath() - _, err := f.Stat(fpath) + id := konf.IDFromProcessID(cmd.Process.Pid) + fpath := sm.ActivePathFromID(id) + _, err := sm.Fs.Stat(fpath) if err != nil { if errors.Is(err, fs.ErrNotExist) { @@ -142,8 +147,9 @@ func TestCleanLeftOvers(t *testing.T) { } for _, cmd := range cmdsStopped { - fpath := konf.IDFromProcessID(cmd.Process.Pid).ActivePath() - _, err := f.Stat(fpath) + id := konf.IDFromProcessID(cmd.Process.Pid) + fpath := sm.ActivePathFromID(id) + _, err := sm.Fs.Stat(fpath) if !errors.Is(err, fs.ErrNotExist) { t.Fatalf("Unexpected error occurred: '%s'", err) @@ -159,12 +165,11 @@ func TestCleanLeftOvers(t *testing.T) { } -func mixedFSWithAllProcs(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { +func mixedFSWithAllProcs(t *testing.T, sm *store.Storemanager) (cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { // we are simulating other instances of konf here numOfConfs := 3 - fs = afero.NewMemMapFs() - sm := testhelper.SampleKonfManager{} + skm := testhelper.SampleKonfManager{} for i := 1; i <= numOfConfs; i++ { // set sleep to an extremely high number as the argument "infinity" does not exist in all versions of the util @@ -176,15 +181,15 @@ func mixedFSWithAllProcs(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cm } pid := cmd.Process.Pid cmdsRunning = append(cmdsRunning, cmd) - afero.WriteFile(fs, konf.IDFromProcessID(pid).ActivePath(), []byte(sm.SingleClusterSingleContextEU()), utils.KonfPerm) + afero.WriteFile(sm.Fs, sm.ActivePathFromID(konf.IDFromProcessID(pid)), []byte(skm.SingleClusterSingleContextEU()), utils.KonfPerm) } - return fs, cmdsRunning, nil + return cmdsRunning, nil } // returns a state where there are more fs files than cmds -func mixedFSIncompleteProcs(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { - fs, cmdsRunning, cmdsStopped = mixedFSWithAllProcs(t) +func mixedFSIncompleteProcs(t *testing.T, sm *store.Storemanager) (cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { + cmdsRunning, cmdsStopped = mixedFSWithAllProcs(t, sm) cmdToKill := cmdsRunning[0] origPID := cmdToKill.Process.Pid @@ -210,22 +215,22 @@ func mixedFSIncompleteProcs(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cmdsStopped = append(cmdsStopped, cmdsRunning[0]) cmdsRunning = cmdsRunning[1:] - return fs, cmdsRunning, cmdsStopped + return cmdsRunning, cmdsStopped } -func mixedFSDirtyDir(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { - fs, cmdsRunning, cmdsStopped = mixedFSIncompleteProcs(t) +func mixedFSDirtyDir(t *testing.T, sm *store.Storemanager) (cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { + cmdsRunning, cmdsStopped = mixedFSIncompleteProcs(t, sm) - afero.WriteFile(fs, konf.KonfID("/not-a-valid-process-id").ActivePath(), []byte{}, utils.KonfPerm) + id := konf.KonfID("/not-a-valid-process-id") + afero.WriteFile(sm.Fs, sm.ActivePathFromID(id), []byte{}, utils.KonfPerm) - return fs, cmdsRunning, cmdsStopped + return cmdsRunning, cmdsStopped } -func emptyFS(t *testing.T) (fs afero.Fs, cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { - fs = afero.NewMemMapFs() - - return fs, nil, nil +func emptyFS(t *testing.T, sm *store.Storemanager) (cmdsRunning []*exec.Cmd, cmdsStopped []*exec.Cmd) { + // no op + return nil, nil } func cleanUpRunningCmds(t *testing.T, cmds []*exec.Cmd) { diff --git a/cmd/delete.go b/cmd/delete.go index ea36231..7688ee9 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -79,10 +79,11 @@ func (c *deleteCmd) delete(cmd *cobra.Command, args []string) error { } func deleteKonfWithID(sm *store.Storemanager, id konf.KonfID) error { - if err := sm.Fs.Remove(id.StorePath()); err != nil { + path := sm.StorePathFromID(id) + if err := sm.Fs.Remove(path); err != nil { return err } - log.Info("Successfully deleted konf %q at %q", id, id.StorePath()) + log.Info("Successfully deleted konf %q at %q", id, path) return nil } diff --git a/cmd/import.go b/cmd/import.go index 1e440eb..39dd2b4 100644 --- a/cmd/import.go +++ b/cmd/import.go @@ -98,7 +98,8 @@ func (c *importCmd) importf(cmd *cobra.Command, args []string) error { if err != nil { return err } - log.Info("Imported konf from %q successfully into %q\n", k.ImportPath, k.Konf.StorePath) + storePath := c.sm.StorePathFromID(k.Konf.Id) + log.Info("Imported konf from %q successfully into %q\n", k.ImportPath, storePath) } if c.move { diff --git a/cmd/set.go b/cmd/set.go index 1d1f3f2..26e6544 100644 --- a/cmd/set.go +++ b/cmd/set.go @@ -25,7 +25,7 @@ type setCmd struct { func newSetCommand() *setCmd { fs := afero.NewOsFs() - sm := &store.Storemanager{Fs: fs, Activedir: config.ActiveDir(), Storedir: config.StoreDir()} + sm := &store.Storemanager{Fs: fs, Activedir: config.ActiveDir(), Storedir: config.StoreDir(), LatestKonfPath: config.LatestKonfFilePath()} sc := &setCmd{ sm: sm, } @@ -61,7 +61,7 @@ func (c *setCmd) set(cmd *cobra.Command, args []string) error { return err } } else if args[0] == "-" { - id, err = idOfLatestKonf(c.sm.Fs) + id, err = idOfLatestKonf(c.sm) if err != nil { return err } @@ -73,7 +73,7 @@ func (c *setCmd) set(cmd *cobra.Command, args []string) error { if err != nil { return err } - err = saveLatestKonf(c.sm.Fs, id) + err = saveLatestKonf(c.sm, id) if err != nil { return fmt.Errorf("could not save latest konf. As a result 'konf set -' might not work: %q ", err) } @@ -135,8 +135,8 @@ func selectSingleKonf(sm *store.Storemanager, pf prompt.RunFunc) (konf.KonfID, e return konf.IDFromClusterAndContext(sel.Cluster, sel.Context), nil } -func idOfLatestKonf(f afero.Fs) (konf.KonfID, error) { - b, err := afero.ReadFile(f, config.LatestKonfFilePath()) +func idOfLatestKonf(sm *store.Storemanager) (konf.KonfID, error) { + b, err := afero.ReadFile(sm.Fs, sm.LatestKonfPath) if err != nil { if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("could not select latest konf, because no konf was yet set") @@ -164,8 +164,8 @@ func setContext(id konf.KonfID, f afero.Fs) (string, error) { } -func saveLatestKonf(f afero.Fs, id konf.KonfID) error { - return afero.WriteFile(f, config.LatestKonfFilePath(), []byte(id), utils.KonfPerm) +func saveLatestKonf(sm *store.Storemanager, id konf.KonfID) error { + return afero.WriteFile(sm.Fs, sm.LatestKonfPath, []byte(id), utils.KonfPerm) } func createSetPrompt(options []*store.Metadata) *promptui.Select { diff --git a/cmd/set_test.go b/cmd/set_test.go index 9336a45..3e32f50 100644 --- a/cmd/set_test.go +++ b/cmd/set_test.go @@ -44,7 +44,8 @@ func TestSelectLastKonf(t *testing.T) { for name, tc := range tt { t.Run(name, func(t *testing.T) { - id, err := idOfLatestKonf(tc.FSCreator()) + sm := &store.Storemanager{Fs: tc.FSCreator(), Activedir: activeDir, Storedir: storeDir, LatestKonfPath: latestKonfPath} + id, err := idOfLatestKonf(sm) if !testhelper.EqualError(tc.ExpError, err) { t.Errorf("Want error %q, got %q", tc.ExpError, err) @@ -107,7 +108,8 @@ func TestSaveLatestKonf(t *testing.T) { expID := konf.KonfID("context_cluster") f := afero.NewMemMapFs() - err := saveLatestKonf(f, expID) + sm := &store.Storemanager{Fs: f, LatestKonfPath: expFile} + err := saveLatestKonf(sm, expID) if err != nil { t.Errorf("Could not save last konf: %q", err) } @@ -195,7 +197,7 @@ func TestSelectContext(t *testing.T) { activeDir := "./konf/active" fm := testhelper.FilesystemManager{Storedir: storeDir, Activedir: activeDir} f := testhelper.FSWithFiles(fm.StoreDir, fm.SingleClusterSingleContextEU, fm.SingleClusterSingleContextASIA)() - sm := &store.Storemanager{Fs: f, Activedir: config.ActiveDir(), Storedir: config.StoreDir()} + sm := &store.Storemanager{Fs: f, Activedir: activeDir, Storedir: storeDir} // cases // - invalid selection diff --git a/config/config.go b/config/config.go index 1ebfe8b..1a5a74d 100644 --- a/config/config.go +++ b/config/config.go @@ -12,12 +12,10 @@ type Config struct { Silent bool } -// This is mainly used to provide some sane and lively defaults for unit tests -// TODO with the new config system in place, it might make sense to rework all the test-cases and remove the "./konf" reference +// ensure that a default config exists so we don't run into nil pointer exceptions +// TODO remove this as soon as we got rid of id.StorePath and id.ActivePath func init() { - curConf = &Config{ - KonfDir: "./konf", - } + curConf = &Config{} } // ConfFromHomeDir returns an initialized config based on the users HomeDir diff --git a/go.mod b/go.mod index a92619d..42b0720 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( k8s.io/api v0.22.3 k8s.io/apimachinery v0.22.3 k8s.io/client-go v0.22.3 + k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a sigs.k8s.io/yaml v1.2.0 ) @@ -44,6 +45,5 @@ require ( gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect k8s.io/klog/v2 v2.9.0 // indirect k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e // indirect - k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a // indirect sigs.k8s.io/structured-merge-diff/v4 v4.1.2 // indirect ) diff --git a/konf/id_test.go b/konf/id_test.go index feb5ffb..6eebfb8 100644 --- a/konf/id_test.go +++ b/konf/id_test.go @@ -24,23 +24,6 @@ var validCombos = []struct { {"this:would:break:on:windows", "danger", "this-would-break-on-windows_danger"}, } -func TestPathForID(t *testing.T) { - for _, co := range validCombos { - resStore := co.id.StorePath() - expStore := fmt.Sprintf("./konf/store/%s.yaml", co.id) - if resStore != expStore { - t.Errorf("Exp StorePath %q, got %q", expStore, resStore) - } - - resActive := co.id.ActivePath() - expActive := fmt.Sprintf("./konf/active/%s.yaml", co.id) - if resActive != expActive { - t.Errorf("Exp ActivePath %q, got %q", expActive, resActive) - } - } - -} - func TestIDFromClusterAndContext(t *testing.T) { for _, co := range validCombos { res := IDFromClusterAndContext(co.cluster, co.context) diff --git a/konf/konfig.go b/konf/konfig.go index 4b41db6..5af70d8 100644 --- a/konf/konfig.go +++ b/konf/konfig.go @@ -7,5 +7,4 @@ import ( type Konfig struct { Id KonfID Kubeconfig k8s.Config - StorePath string } diff --git a/konf/split.go b/konf/split.go index e1621f2..cb833ed 100644 --- a/konf/split.go +++ b/konf/split.go @@ -51,8 +51,6 @@ func KonfsFromKubeconfig(kubeconfig io.Reader) (konfs []*Konfig, err error) { var k Konfig id := IDFromClusterAndContext(cluster.Name, curCon.Name) - // TODO need to remove this. StorePath should only be setable by store pkg later on - k.StorePath = id.StorePath() k.Id = id k.Kubeconfig.AuthInfos = append(k.Kubeconfig.AuthInfos, user) k.Kubeconfig.Clusters = append(k.Kubeconfig.Clusters, cluster) diff --git a/konf/split_test.go b/konf/split_test.go index 5551716..134be53 100644 --- a/konf/split_test.go +++ b/konf/split_test.go @@ -81,8 +81,7 @@ func TestKonfsFromKubeConfig(t *testing.T) { kubeconfig: singleClusterSingleContext, expKonfs: []*Konfig{ { - Id: "dev-eu_dev-eu-1", - StorePath: "./konf/store/dev-eu_dev-eu-1.yaml", + Id: "dev-eu_dev-eu-1", Kubeconfig: k8s.Config{ APIVersion: "v1", Kind: "Config", @@ -118,8 +117,7 @@ func TestKonfsFromKubeConfig(t *testing.T) { kubeconfig: multiClusterMultiContext, expKonfs: []*Konfig{ { - Id: "dev-asia_dev-asia-1", - StorePath: "./konf/store/dev-asia_dev-asia-1.yaml", + Id: "dev-asia_dev-asia-1", Kubeconfig: k8s.Config{ APIVersion: "v1", Kind: "Config", @@ -150,8 +148,7 @@ func TestKonfsFromKubeConfig(t *testing.T) { }, }, { - Id: "dev-eu_dev-eu-1", - StorePath: "./konf/store/dev-eu_dev-eu-1.yaml", + Id: "dev-eu_dev-eu-1", Kubeconfig: k8s.Config{ APIVersion: "v1", Kind: "Config", diff --git a/store/store.go b/store/store.go index 3e14e39..c4103f6 100644 --- a/store/store.go +++ b/store/store.go @@ -24,9 +24,10 @@ type Metadata struct { } type Storemanager struct { - Activedir string - Storedir string - Fs afero.Fs + Activedir string + Storedir string + LatestKonfPath string + Fs afero.Fs } // FetchAllKonfs retrieves metadata for all konfs currently in the store diff --git a/store/store_test.go b/store/store_test.go index 39b705b..5ebb9dd 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -323,8 +323,7 @@ users: expPath := "./konf/store/dev-eu_dev-eu-1.yaml" var devEUControlGroup = &konf.Konfig{ - Id: konf.IDFromClusterAndContext("dev-eu-1", "dev-eu"), - StorePath: konf.IDFromClusterAndContext("dev-eu-1", "dev-eu").StorePath(), + Id: konf.IDFromClusterAndContext("dev-eu-1", "dev-eu"), Kubeconfig: k8s.Config{ APIVersion: "v1", Kind: "Config", @@ -364,7 +363,7 @@ users: t.Errorf("Exp path to be %q, but got %q", expPath, p) } - b, err := afero.ReadFile(f, devEUControlGroup.StorePath) + b, err := afero.ReadFile(f, expPath) if err != nil { t.Errorf("Exp read in file without any issues, but got %q", err) } diff --git a/utils/dir_test.go b/utils/dir_test.go index 5b7e4e4..10ebc74 100644 --- a/utils/dir_test.go +++ b/utils/dir_test.go @@ -3,12 +3,22 @@ package utils import ( "testing" + "github.com/simontheleg/konf-go/config" "github.com/spf13/afero" ) func TestEnsureDir(t *testing.T) { + // since ensureDir is being run long before any storemanager is created, + // we need to manually set a config here + c := &config.Config{ + KonfDir: "./konf", + } + config.InitWithOverrides(c) f := afero.NewMemMapFs() - EnsureDir(f) + err := EnsureDir(f) + if err != nil { + t.Errorf("Unexpected error while running EnsureDir: %q", err) + } r, err := f.Stat("./konf/active") if err != nil {