From b1c688d8a7fe573395ee0e671fd2d85125188055 Mon Sep 17 00:00:00 2001 From: Grigoriy Mikhalkin Date: Fri, 14 Jan 2022 19:15:23 +0100 Subject: [PATCH 1/2] switches refactoring --- cmdutils/switches/switches.go | 90 ++++++++++++++++++++++-------- cmdutils/switches/switches_test.go | 52 ++++++++++------- 2 files changed, 97 insertions(+), 45 deletions(-) diff --git a/cmdutils/switches/switches.go b/cmdutils/switches/switches.go index 643c917..fa9ce25 100644 --- a/cmdutils/switches/switches.go +++ b/cmdutils/switches/switches.go @@ -19,6 +19,7 @@ package switches import ( "encoding/csv" "fmt" + "sort" "strings" "sigs.k8s.io/kustomize/kyaml/sets" @@ -31,15 +32,25 @@ const ( ) type Switches struct { + defaults map[string]bool settings map[string]bool } -// New creates an instance of Switches -func New(settings []string) *Switches { - s := &Switches{ +// New creates an instance of Switches and returns the pointer to it +func New(settings ...string) *Switches { + s := Make(settings...) + return &s +} + +// Make creates an instance of Switches +// Same as New but returns copy of a struct, not a pointer +func Make(settings ...string) Switches { + s := Switches{ + defaults: make(map[string]bool), settings: make(map[string]bool), } - s.setSettings(settings) + + s.defaults = s.prepareSettings(settings) return s } @@ -48,8 +59,28 @@ func Disable(name string) string { return disablePrefix + name } -func (s *Switches) String() string { - return fmt.Sprintf("%v", s.settings) +func (s Switches) String() string { + var res string + + vals := make([]string, 0, len(s.defaults)) + for v := range s.defaults { + vals = append(vals, v) + } + + sort.Strings(vals) + for _, v := range vals { + if res != "" { + res += "," + } + + if s.settings[v] { + res += v + } else { + res += "-" + v + } + } + + return res } func (s *Switches) Set(val string) error { @@ -70,7 +101,7 @@ func (s *Switches) Set(val string) error { // Validate that all specified controllers are known for _, v := range settings { trimmed := strings.TrimPrefix(v, disablePrefix) - if _, ok := s.settings[trimmed]; trimmed != All && !ok { + if _, ok := s.defaults[trimmed]; trimmed != All && !ok { return fmt.Errorf("unknown item: %s", trimmed) } } @@ -78,17 +109,17 @@ func (s *Switches) Set(val string) error { settings = []string{""} } - s.setSettings(settings) + s.settings = s.prepareSettings(settings) return nil } // Enabled checks if item is enabled -func (s *Switches) Enabled(name string) bool { +func (s Switches) Enabled(name string) bool { return s.settings[name] } // All returns names of all items set in settings -func (s *Switches) All() sets.String { +func (s Switches) All() sets.String { names := make(sets.String, len(s.settings)) for k := range s.settings { names.Insert(k) @@ -97,10 +128,22 @@ func (s *Switches) All() sets.String { return names } +// EnabledByDefault returns names of all enabled items +func (s Switches) EnabledByDefault() sets.String { + names := make(sets.String) + for k, enabled := range s.defaults { + if enabled { + names.Insert(k) + } + } + + return names +} + // DisabledByDefault returns names of all disabled items -func (s *Switches) DisabledByDefault() sets.String { +func (s Switches) DisabledByDefault() sets.String { names := make(sets.String) - for k, enabled := range s.settings { + for k, enabled := range s.defaults { if !enabled { names.Insert(k) } @@ -109,33 +152,32 @@ func (s *Switches) DisabledByDefault() sets.String { return names } -func (s *Switches) Type() string { - return "Switches" +func (s Switches) Type() string { + return "strings" } -func (s *Switches) setSettings(settings []string) { +func (s Switches) prepareSettings(settings []string) (res map[string]bool) { + res = make(map[string]bool) + if len(settings) == 1 && settings[0] == "" { return } - var isDefault bool for _, v := range settings { if v == All { - isDefault = true + for k, v := range s.defaults { + res[k] = v + } break } } - if !isDefault { - for k := range s.settings { - s.settings[k] = false - } - } - for _, v := range settings { if v == All { continue } - s.settings[strings.TrimPrefix(v, disablePrefix)] = !strings.HasPrefix(v, disablePrefix) + res[strings.TrimPrefix(v, disablePrefix)] = !strings.HasPrefix(v, disablePrefix) } + + return } diff --git a/cmdutils/switches/switches_test.go b/cmdutils/switches/switches_test.go index 98e4ff2..aaff319 100644 --- a/cmdutils/switches/switches_test.go +++ b/cmdutils/switches/switches_test.go @@ -16,54 +16,64 @@ package switches import ( "flag" + "github.com/spf13/pflag" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/spf13/pflag" "sigs.k8s.io/kustomize/kyaml/sets" ) var _ = Describe("CMD Switches", func() { Context("Testing Switches interface", func() { It("should disable runner", func() { - s := New([]string{"runner-a", Disable("runner-b")}) + s := New("runner-a", "runner-b") + Expect(s.Set("*,-runner-b")).ToNot(HaveOccurred()) Expect(s.Enabled("runner-a")).To(BeTrue()) Expect(s.Enabled("runner-b")).To(BeFalse()) }) It("should return all items", func() { - s := New([]string{"runner-a", Disable("runner-b")}) + s := New("runner-a", "runner-b") + Expect(s.Set("*,-runner-b")).ToNot(HaveOccurred()) expected := make(sets.String) expected.Insert("runner-a", "runner-b") Expect(s.All()).To(Equal(expected)) }) + It("should return all enabled items", func() { + s := New("runner-a", Disable("runner-b")) + + expected := make(sets.String) + expected.Insert("runner-a") + Expect(s.EnabledByDefault()).To(Equal(expected)) + }) It("should return all disabled items", func() { - s := New([]string{"runner-a", Disable("runner-b")}) + s := New("runner-a", Disable("runner-b")) expected := make(sets.String) expected.Insert("runner-b") Expect(s.DisabledByDefault()).To(Equal(expected)) }) It("should return string", func() { - s := New([]string{"runner-a", Disable("runner-b")}) - - Expect(s.String()).To(Equal("map[runner-a:true runner-b:false]")) + s := New("runner-a", "runner-b") + Expect(s.Set("*,-runner-b")).ToNot(HaveOccurred()) + Expect(s.String()).To(Equal("runner-a,-runner-b")) }) }) Context("Testing flag package behavior", func() { - It("should keep default settings when no flag is passed", func() { + It("should disable all controllers when no flag is passed", func() { fs := flag.NewFlagSet("", flag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{})).NotTo(HaveOccurred()) - Expect(controllers.Enabled("runner-a")).To(BeTrue()) + Expect(controllers.Enabled("runner-a")).To(BeFalse()) Expect(controllers.Enabled("runner-b")).To(BeFalse()) - Expect(controllers.Enabled("runner-c")).To(BeTrue()) + Expect(controllers.Enabled("runner-c")).To(BeFalse()) }) It("should keep default settings when * is passed", func() { fs := flag.NewFlagSet("", flag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=*"})).NotTo(HaveOccurred()) @@ -73,7 +83,7 @@ var _ = Describe("CMD Switches", func() { }) It("should override default settings", func() { fs := flag.NewFlagSet("", flag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=runner-a,-runner-c"})).NotTo(HaveOccurred()) @@ -83,7 +93,7 @@ var _ = Describe("CMD Switches", func() { }) It("should override some of default settings", func() { fs := flag.NewFlagSet("", flag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=*,-runner-a"})).NotTo(HaveOccurred()) @@ -94,19 +104,19 @@ var _ = Describe("CMD Switches", func() { }) Context("Testing pflag package behavior", func() { - It("should keep default settings when no flag is passed", func() { + It("should disable all controllers when no flag is passed", func() { fs := pflag.NewFlagSet("", pflag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{})).NotTo(HaveOccurred()) - Expect(controllers.Enabled("runner-a")).To(BeTrue()) + Expect(controllers.Enabled("runner-a")).To(BeFalse()) Expect(controllers.Enabled("runner-b")).To(BeFalse()) - Expect(controllers.Enabled("runner-c")).To(BeTrue()) + Expect(controllers.Enabled("runner-c")).To(BeFalse()) }) It("should keep default settings when * is passed", func() { fs := pflag.NewFlagSet("", pflag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=*"})).NotTo(HaveOccurred()) @@ -116,7 +126,7 @@ var _ = Describe("CMD Switches", func() { }) It("should override default settings", func() { fs := pflag.NewFlagSet("", pflag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=runner-a,-runner-c"})).NotTo(HaveOccurred()) @@ -126,7 +136,7 @@ var _ = Describe("CMD Switches", func() { }) It("should override some of default settings", func() { fs := pflag.NewFlagSet("", pflag.ExitOnError) - controllers := New([]string{"runner-a", Disable("runner-b"), "runner-c"}) + controllers := New("runner-a", Disable("runner-b"), "runner-c") fs.Var(controllers, "controllers", "") Expect(fs.Parse([]string{"--controllers=*,-runner-a"})).NotTo(HaveOccurred()) From 9355dfc2e7777d3a6861b59034d654105aca29b1 Mon Sep 17 00:00:00 2001 From: Grigoriy Mikhalkin Date: Tue, 18 Jan 2022 12:12:34 +0100 Subject: [PATCH 2/2] refactoring pt2 --- cmdutils/switches/switches.go | 22 +++++++++++----------- cmdutils/switches/switches_test.go | 14 +++++--------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/cmdutils/switches/switches.go b/cmdutils/switches/switches.go index fa9ce25..331c871 100644 --- a/cmdutils/switches/switches.go +++ b/cmdutils/switches/switches.go @@ -22,7 +22,7 @@ import ( "sort" "strings" - "sigs.k8s.io/kustomize/kyaml/sets" + "k8s.io/apimachinery/pkg/util/sets" ) const ( @@ -59,7 +59,7 @@ func Disable(name string) string { return disablePrefix + name } -func (s Switches) String() string { +func (s *Switches) String() string { var res string vals := make([]string, 0, len(s.defaults)) @@ -114,13 +114,13 @@ func (s *Switches) Set(val string) error { } // Enabled checks if item is enabled -func (s Switches) Enabled(name string) bool { +func (s *Switches) Enabled(name string) bool { return s.settings[name] } // All returns names of all items set in settings -func (s Switches) All() sets.String { - names := make(sets.String, len(s.settings)) +func (s *Switches) All() sets.String { + names := sets.NewString() for k := range s.settings { names.Insert(k) } @@ -129,8 +129,8 @@ func (s Switches) All() sets.String { } // EnabledByDefault returns names of all enabled items -func (s Switches) EnabledByDefault() sets.String { - names := make(sets.String) +func (s *Switches) EnabledByDefault() sets.String { + names := sets.NewString() for k, enabled := range s.defaults { if enabled { names.Insert(k) @@ -141,8 +141,8 @@ func (s Switches) EnabledByDefault() sets.String { } // DisabledByDefault returns names of all disabled items -func (s Switches) DisabledByDefault() sets.String { - names := make(sets.String) +func (s *Switches) DisabledByDefault() sets.String { + names := sets.NewString() for k, enabled := range s.defaults { if !enabled { names.Insert(k) @@ -152,11 +152,11 @@ func (s Switches) DisabledByDefault() sets.String { return names } -func (s Switches) Type() string { +func (s *Switches) Type() string { return "strings" } -func (s Switches) prepareSettings(settings []string) (res map[string]bool) { +func (s *Switches) prepareSettings(settings []string) (res map[string]bool) { res = make(map[string]bool) if len(settings) == 1 && settings[0] == "" { diff --git a/cmdutils/switches/switches_test.go b/cmdutils/switches/switches_test.go index aaff319..9e65f4a 100644 --- a/cmdutils/switches/switches_test.go +++ b/cmdutils/switches/switches_test.go @@ -16,11 +16,10 @@ package switches import ( "flag" - "github.com/spf13/pflag" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "sigs.k8s.io/kustomize/kyaml/sets" + "github.com/spf13/pflag" + "k8s.io/apimachinery/pkg/util/sets" ) var _ = Describe("CMD Switches", func() { @@ -35,22 +34,19 @@ var _ = Describe("CMD Switches", func() { s := New("runner-a", "runner-b") Expect(s.Set("*,-runner-b")).ToNot(HaveOccurred()) - expected := make(sets.String) - expected.Insert("runner-a", "runner-b") + expected := sets.NewString("runner-a", "runner-b") Expect(s.All()).To(Equal(expected)) }) It("should return all enabled items", func() { s := New("runner-a", Disable("runner-b")) - expected := make(sets.String) - expected.Insert("runner-a") + expected := sets.NewString("runner-a") Expect(s.EnabledByDefault()).To(Equal(expected)) }) It("should return all disabled items", func() { s := New("runner-a", Disable("runner-b")) - expected := make(sets.String) - expected.Insert("runner-b") + expected := sets.NewString("runner-b") Expect(s.DisabledByDefault()).To(Equal(expected)) }) It("should return string", func() {