From 1273c6493b7e59b1018a22da1d78015c9169c8de Mon Sep 17 00:00:00 2001 From: ferhat elmas Date: Fri, 13 Dec 2019 02:18:31 +0100 Subject: [PATCH 1/5] Some refactoring improvements * don't shadow imports or builtins such as `os` and `close` respectively. * use wrapper ReplaceAll * combine types in func calls if same type --- internal/download/downloader.go | 12 ++++---- internal/download/verifier.go | 4 +-- internal/installation/install.go | 6 ++-- internal/installation/install_test.go | 4 +-- internal/installation/platform.go | 40 ++++++++++++++++--------- internal/installation/platform_test.go | 32 ++++++++++---------- internal/installation/upgrade.go | 4 +-- internal/receiptsmigration/migration.go | 4 +-- 8 files changed, 59 insertions(+), 47 deletions(-) diff --git a/internal/download/downloader.go b/internal/download/downloader.go index 3ea5c262..2890b3a5 100644 --- a/internal/download/downloader.go +++ b/internal/download/downloader.go @@ -80,16 +80,16 @@ func extractZIP(targetDir string, read io.ReaderAt, size int64) error { src.Close() return errors.Wrap(err, "can't create file in zip destination dir") } - close := func() { + closeAll := func() { src.Close() dst.Close() } if _, err := io.Copy(dst, src); err != nil { - close() + closeAll() return errors.Wrap(err, "can't copy content to zip destination file") } - close() + closeAll() } return nil @@ -142,12 +142,12 @@ func extractTARGZ(targetDir string, at io.ReaderAt, size int64) error { if err != nil { return errors.Wrapf(err, "failed to create file %q", path) } - close := func() { f.Close() } + if _, err := io.Copy(f, tr); err != nil { - close() + f.Close() return errors.Wrapf(err, "failed to copy %q from tar into file", hdr.Name) } - close() + f.Close() default: return errors.Errorf("unable to handle file type %d for %q in tar", hdr.Typeflag, hdr.Name) } diff --git a/internal/download/verifier.go b/internal/download/verifier.go index 8db95a3a..f4adb251 100644 --- a/internal/download/verifier.go +++ b/internal/download/verifier.go @@ -39,8 +39,8 @@ type sha256Verifier struct { } // NewSha256Verifier creates a Verifier that tests against the given hash. -func NewSha256Verifier(hash string) Verifier { - raw, _ := hex.DecodeString(hash) +func NewSha256Verifier(hashed string) Verifier { + raw, _ := hex.DecodeString(hashed) return sha256Verifier{ Hash: sha256.New(), wantedHash: raw, diff --git a/internal/installation/install.go b/internal/installation/install.go index 762f13f8..c2837780 100644 --- a/internal/installation/install.go +++ b/internal/installation/install.go @@ -186,7 +186,7 @@ func Uninstall(p environment.Paths, name string) error { return errors.Wrapf(err, "could not remove plugin receipt %q", pluginReceiptPath) } -func createOrUpdateLink(binDir string, binary string, plugin string) error { +func createOrUpdateLink(binDir, binary, plugin string) error { dst := filepath.Join(binDir, pluginNameToBin(plugin, IsWindows())) if err := removeLink(dst); err != nil { @@ -238,7 +238,7 @@ func IsWindows() bool { // pluginNameToBin creates the name of the symlink file for the plugin name. // It converts dashes to underscores. func pluginNameToBin(name string, isWindows bool) string { - name = strings.Replace(name, "-", "_", -1) + name = strings.ReplaceAll(name, "-", "_") name = "kubectl-" + name if isWindows { name += ".exe" @@ -247,7 +247,7 @@ func pluginNameToBin(name string, isWindows bool) string { } // CleanupStaleKrewInstallations removes the versions that aren't the current version. -func CleanupStaleKrewInstallations(dir string, currentVersion string) error { +func CleanupStaleKrewInstallations(dir, currentVersion string) error { ls, err := ioutil.ReadDir(dir) if err != nil { return errors.Wrap(err, "failed to read krew store directory") diff --git a/internal/installation/install_test.go b/internal/installation/install_test.go index 3d2aee82..8a10014e 100644 --- a/internal/installation/install_test.go +++ b/internal/installation/install_test.go @@ -299,8 +299,8 @@ func Test_applyDefaults(t *testing.T) { } func TestCleanupStaleKrewInstallations(t *testing.T) { - dir, close := testutil.NewTempDir(t) - defer close() + dir, cleanup := testutil.NewTempDir(t) + defer cleanup() testFiles := []string{ "dir1/f1.txt", diff --git a/internal/installation/platform.go b/internal/installation/platform.go index 68754456..71f8dd17 100644 --- a/internal/installation/platform.go +++ b/internal/installation/platform.go @@ -15,6 +15,7 @@ package installation import ( + "fmt" "os" "runtime" @@ -27,18 +28,17 @@ import ( ) // GetMatchingPlatform finds the platform spec in the specified plugin that -// matches the OS/arch of the current machine (can be overridden via KREW_OS +// matches the os/arch of the current machine (can be overridden via KREW_OS // and/or KREW_ARCH). func GetMatchingPlatform(platforms []index.Platform) (index.Platform, bool, error) { - os, arch := osArch() - return matchPlatform(platforms, os, arch) + return matchPlatform(platforms, osArch()) } // matchPlatform returns the first matching platform to given os/arch. -func matchPlatform(platforms []index.Platform, os, arch string) (index.Platform, bool, error) { +func matchPlatform(platforms []index.Platform, env goOSArch) (index.Platform, bool, error) { envLabels := labels.Set{ - "os": os, - "arch": arch, + "os": env.os, + "arch": env.arch, } klog.V(2).Infof("Matching platform for labels(%v)", envLabels) @@ -55,16 +55,28 @@ func matchPlatform(platforms []index.Platform, os, arch string) (index.Platform, return index.Platform{}, false, nil } +type goOSArch struct { + os, arch string +} + +// String converts environment into a string +func (env goOSArch) String() string { + return fmt.Sprintf("(%s/%s)", env.os, env.arch) +} + // osArch returns the OS/arch combination to be used on the current system. It // can be overridden by setting KREW_OS and/or KREW_ARCH environment variables. -func osArch() (string, string) { - goos, goarch := runtime.GOOS, runtime.GOARCH - envOS, envArch := os.Getenv("KREW_OS"), os.Getenv("KREW_ARCH") - if envOS != "" { - goos = envOS +func osArch() goOSArch { + return goOSArch{ + os: getEnvOrDefault("KREW_OS", runtime.GOOS), + arch: getEnvOrDefault("KREW_ARCH", runtime.GOARCH), } - if envArch != "" { - goarch = envArch +} + +func getEnvOrDefault(env, absent string) string { + v := os.Getenv(env) + if v != "" { + return v } - return goos, goarch + return absent } diff --git a/internal/installation/platform_test.go b/internal/installation/platform_test.go index 874334ba..8b9aed53 100644 --- a/internal/installation/platform_test.go +++ b/internal/installation/platform_test.go @@ -27,12 +27,12 @@ import ( func Test_osArch(t *testing.T) { inOS, inArch := runtime.GOOS, runtime.GOARCH - outOS, outArch := osArch() - if inOS != outOS { - t.Errorf("returned OS=%q; expected=%q", outOS, inOS) + out := osArch() + if inOS != out.os { + t.Errorf("returned OS=%q; expected=%q", out.os, inOS) } - if inArch != outArch { - t.Errorf("returned Arch=%q; expected=%q", outArch, inArch) + if inArch != out.arch { + t.Errorf("returned Arch=%q; expected=%q", out.arch, inArch) } } @@ -45,22 +45,22 @@ func Test_osArch_override(t *testing.T) { os.Unsetenv("KREW_OS") }() - outOS, outArch := osArch() - if customOS != outOS { - t.Errorf("returned OS=%q; expected=%q", outOS, customOS) + out := osArch() + if customOS != out.os { + t.Errorf("returned OS=%q; expected=%q", out.os, customOS) } - if customArch != outArch { - t.Errorf("returned Arch=%q; expected=%q", outArch, customArch) + if customArch != out.arch { + t.Errorf("returned Arch=%q; expected=%q", out.arch, customArch) } } func Test_matchPlatform(t *testing.T) { - const targetOS, targetArch = "foo", "amd64" - matchingPlatform := testutil.NewPlatform().WithOSArch(targetOS, targetArch).V() - differentOS := testutil.NewPlatform().WithOSArch("other", targetArch).V() - differentArch := testutil.NewPlatform().WithOSArch(targetOS, "other").V() + target := goOSArch{os: "foo", arch: "amd64"} + matchingPlatform := testutil.NewPlatform().WithOSArch(target.os, target.arch).V() + differentOS := testutil.NewPlatform().WithOSArch("other", target.arch).V() + differentArch := testutil.NewPlatform().WithOSArch(target.os, "other").V() - p, ok, err := matchPlatform([]index.Platform{differentOS, differentArch, matchingPlatform}, targetOS, targetArch) + p, ok, err := matchPlatform([]index.Platform{differentOS, differentArch, matchingPlatform}, target) if err != nil { t.Fatal(err) } @@ -71,7 +71,7 @@ func Test_matchPlatform(t *testing.T) { t.Fatalf("got a different object from the matching platform:\n%s", diff) } - _, ok, err = matchPlatform([]index.Platform{differentOS, differentArch}, targetOS, targetArch) + _, ok, err = matchPlatform([]index.Platform{differentOS, differentArch}, target) if err != nil { t.Fatal(err) } diff --git a/internal/installation/upgrade.go b/internal/installation/upgrade.go index cfa853be..e01af4ee 100644 --- a/internal/installation/upgrade.go +++ b/internal/installation/upgrade.go @@ -48,8 +48,8 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error { return errors.Wrap(err, "failed trying to find a matching platform in plugin spec") } if !ok { - os, arch := osArch() - return errors.Errorf("plugin %q does not offer installation for this platform (%s/%s)", plugin.Name, os, arch) + return errors.Errorf("plugin %q does not offer installation for this platform %s", + plugin.Name, osArch()) } newVersion := plugin.Spec.Version diff --git a/internal/receiptsmigration/migration.go b/internal/receiptsmigration/migration.go index 37f6c49f..e0b49a8a 100644 --- a/internal/receiptsmigration/migration.go +++ b/internal/receiptsmigration/migration.go @@ -193,7 +193,7 @@ func removeLink(path string) error { return nil } -/// same as pkg/installation/install.go:186 +// same as pkg/installation/install.go:186 func isWindows() bool { goos := runtime.GOOS if env := os.Getenv("KREW_OS"); env != "" { @@ -206,7 +206,7 @@ func isWindows() bool { // It converts dashes to underscores. // same as pkg/installation/install.go:196 func pluginNameToBin(name string, isWindows bool) string { - name = strings.Replace(name, "-", "_", -1) + name = strings.ReplaceAll(name, "-", "_") name = "kubectl-" + name if isWindows { name += ".exe" From 7a793373c6f2c912f622097126bb8d1e19eea9d6 Mon Sep 17 00:00:00 2001 From: ferhat elmas Date: Fri, 13 Dec 2019 08:06:47 +0100 Subject: [PATCH 2/5] More refactoring and golangci config improvement --- .golangci.yml | 11 +++ .../generate-plugin-overview.go | 2 +- cmd/validate-krew-manifest/main.go | 57 +++++++------ cmd/validate-krew-manifest/main_test.go | 79 +++++++++++++------ go.sum | 1 + internal/installation/platform.go | 27 ++++--- internal/installation/platform_test.go | 28 +++---- internal/installation/upgrade.go | 4 +- 8 files changed, 124 insertions(+), 85 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 0fc5c40c..be0ee523 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,5 +1,16 @@ # all available settings of specific linters linters-settings: + gocritic: + enabled-tags: + - performance + - diagnostic + - style + - experimental + - opinionated + disabled-checks: + - hugeParam + - rangeValCopy + - unnamedResult gofmt: simplify: true goimports: diff --git a/cmd/generate-plugin-overview/generate-plugin-overview.go b/cmd/generate-plugin-overview/generate-plugin-overview.go index 4aebed54..214d8138 100644 --- a/cmd/generate-plugin-overview/generate-plugin-overview.go +++ b/cmd/generate-plugin-overview/generate-plugin-overview.go @@ -55,7 +55,7 @@ tool._ ) var ( - githubRepoPattern = regexp.MustCompile(`.*github.com/([^/]+/[^/#]+)`) + githubRepoPattern = regexp.MustCompile(`.*github\.com/([^/]+/[^/#]+)`) ) func main() { diff --git a/cmd/validate-krew-manifest/main.go b/cmd/validate-krew-manifest/main.go index 0f86a49e..e60b26c9 100644 --- a/cmd/validate-krew-manifest/main.go +++ b/cmd/validate-krew-manifest/main.go @@ -32,6 +32,7 @@ import ( "sigs.k8s.io/krew/internal/index/indexscanner" "sigs.k8s.io/krew/internal/index/validation" + "sigs.k8s.io/krew/internal/installation" "sigs.k8s.io/krew/pkg/constants" "sigs.k8s.io/krew/pkg/index" ) @@ -83,7 +84,7 @@ func validateManifestFile(path string) error { // make sure each platform matches a supported platform for i, p := range p.Spec.Platforms { - if os, arch := findAnyMatchingPlatform(p.Selector); os == "" || arch == "" { + if env := findAnyMatchingPlatform(p.Selector); env.OS == "" || env.Arch == "" { return errors.Errorf("spec.platform[%d]'s selector (%v) doesn't match any supported platforms", i, p.Selector) } } @@ -110,18 +111,16 @@ func validateManifestFile(path string) error { // isOverlappingPlatformSelectors validates if multiple platforms have selectors // that match to a supported pair. func isOverlappingPlatformSelectors(platforms []index.Platform) error { - for _, v := range allPlatforms() { - os, arch := v[0], v[1] - + for _, env := range allPlatforms() { var matchIndex []int for i, p := range platforms { - if selectorMatchesOSArch(p.Selector, os, arch) { + if selectorMatchesOSArch(p.Selector, env) { matchIndex = append(matchIndex, i) } } if len(matchIndex) > 1 { - return errors.Errorf("multiple spec.platforms (at indexes %v) have overlapping selectors that select os=%s/arch=%s", matchIndex, os, arch) + return errors.Errorf("multiple spec.platforms (at indexes %v) have overlapping selectors that select %s", matchIndex, env) } } return nil @@ -130,8 +129,8 @@ func isOverlappingPlatformSelectors(platforms []index.Platform) error { // installPlatformSpec installs the p to a temporary location on disk to verify // by shelling out to external command. func installPlatformSpec(manifestFile string, p index.Platform) error { - goos, goarch := findAnyMatchingPlatform(p.Selector) - if goos == "" || goarch == "" { + env := findAnyMatchingPlatform(p.Selector) + if env.OS == "" || env.Arch == "" { return errors.Errorf("no supported platform matched platform selector: %+v", p.Selector) } @@ -149,33 +148,33 @@ func installPlatformSpec(manifestFile string, p index.Platform) error { cmd.Stdin = nil cmd.Env = []string{ "KREW_ROOT=" + tmpDir, - "KREW_OS=" + goos, - "KREW_ARCH=" + goarch, + "KREW_OS=" + env.OS, + "KREW_ARCH=" + env.Arch, } klog.V(2).Infof("installing plugin with: %+v", cmd.Env) cmd.Env = append(cmd.Env, "PATH="+os.Getenv("PATH")) b, err := cmd.CombinedOutput() if err != nil { - output := strings.Replace(string(b), "\n", "\n\t", -1) + output := strings.ReplaceAll(string(b), "\n", "\n\t") return errors.Wrapf(err, "plugin install command failed: %s", output) } return nil } // findAnyMatchingPlatform finds an pair matches to given selector -func findAnyMatchingPlatform(selector *metav1.LabelSelector) (string, string) { +func findAnyMatchingPlatform(selector *metav1.LabelSelector) installation.GoOSArch { for _, p := range allPlatforms() { - if selectorMatchesOSArch(selector, p[0], p[1]) { - klog.V(4).Infof("%s MATCHED <%s,%s>", selector, p[0], p[1]) - return p[0], p[1] + if selectorMatchesOSArch(selector, p) { + klog.V(4).Infof("%s MATCHED <%s>", selector, p) + return p } - klog.V(4).Infof("%s didn't match <%s,%s>", selector, p[0], p[1]) + klog.V(4).Infof("%s didn't match <%s>", selector, p) } - return "", "" + return installation.GoOSArch{} } -func selectorMatchesOSArch(selector *metav1.LabelSelector, os, arch string) bool { +func selectorMatchesOSArch(selector *metav1.LabelSelector, env installation.GoOSArch) bool { sel, err := metav1.LabelSelectorAsSelector(selector) if err != nil { // this should've been caught by validation.ValidatePlatform() earlier @@ -183,21 +182,21 @@ func selectorMatchesOSArch(selector *metav1.LabelSelector, os, arch string) bool return false } return sel.Matches(labels.Set{ - "os": os, - "arch": arch, + "os": env.OS, + "arch": env.Arch, }) } // allPlatforms returns all pairs krew is supported on. -func allPlatforms() [][2]string { +func allPlatforms() []installation.GoOSArch { // TODO(ahmetb) find a more authoritative source for this list - return [][2]string{ - {"windows", "386"}, - {"windows", "amd64"}, - {"linux", "386"}, - {"linux", "amd64"}, - {"linux", "arm"}, - {"darwin", "386"}, - {"darwin", "amd64"}, + return []installation.GoOSArch{ + {OS: "windows", Arch: "386"}, + {OS: "windows", Arch: "amd64"}, + {OS: "linux", Arch: "386"}, + {OS: "linux", Arch: "amd64"}, + {OS: "linux", Arch: "arm"}, + {OS: "darwin", Arch: "386"}, + {OS: "darwin", Arch: "amd64"}, } } diff --git a/cmd/validate-krew-manifest/main_test.go b/cmd/validate-krew-manifest/main_test.go index d3619eb7..30040ad4 100644 --- a/cmd/validate-krew-manifest/main_test.go +++ b/cmd/validate-krew-manifest/main_test.go @@ -20,9 +20,9 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" + "sigs.k8s.io/krew/internal/installation" "sigs.k8s.io/krew/internal/testutil" "sigs.k8s.io/krew/pkg/index" ) @@ -112,30 +112,57 @@ func TestValidateManifestFile(t *testing.T) { func Test_selectorMatchesOSArch(t *testing.T) { type args struct { selector *metav1.LabelSelector - os string - arch string + env installation.GoOSArch } tests := []struct { name string args args want bool }{ - {"label - no match", args{&metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, "windows", "amd64"}, false}, - {"label - match", args{&metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, "darwin", "amd64"}, true}, - {"expression - no match", args{&metav1.LabelSelector{MatchExpressions: []v1.LabelSelectorRequirement{{ - Key: "os", - Operator: v1.LabelSelectorOpIn, - Values: []string{"darwin", "linux"}, - }}}, "windows", "amd64"}, false}, - {"expression - match", args{&metav1.LabelSelector{MatchExpressions: []v1.LabelSelectorRequirement{{ - Key: "os", - Operator: v1.LabelSelectorOpIn, - Values: []string{"darwin", "linux"}, - }}}, "darwin", "amd64"}, true}, + { + name: "label - no match", + args: args{ + selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, + env: installation.GoOSArch{OS: "windows", Arch: "amd64"}, + }, + want: false, + }, + { + name: "label - match", + args: args{ + selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, + env: installation.GoOSArch{OS: "darwin", Arch: "amd64"}, + }, + want: true, + }, + { + name: "expression - no match", + args: args{ + selector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: "os", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"darwin", "linux"}, + }}}, + env: installation.GoOSArch{OS: "windows", Arch: "amd64"}, + }, + want: false, + }, + { + name: "expression - match", + args: args{ + selector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: "os", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"darwin", "linux"}, + }}}, + env: installation.GoOSArch{OS: "darwin", Arch: "amd64"}, + }, + want: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := selectorMatchesOSArch(tt.args.selector, tt.args.os, tt.args.arch); got != tt.want { + if got := selectorMatchesOSArch(tt.args.selector, tt.args.env); got != tt.want { t.Errorf("selectorMatchesOSArch() = %v, want %v", got, tt.want) } }) @@ -143,24 +170,24 @@ func Test_selectorMatchesOSArch(t *testing.T) { } func Test_findAnyMatchingPlatform(t *testing.T) { - s1 := &v1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}} - o1, a1 := findAnyMatchingPlatform(s1) - if o1 == "" || a1 == "" { + s1 := &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}} + env := findAnyMatchingPlatform(s1) + if env.OS == "" || env.Arch == "" { t.Fatalf("with selector %v, expected os/arch", s1) } - s2 := &v1.LabelSelector{MatchLabels: map[string]string{"os": "non-existing"}} - o2, a2 := findAnyMatchingPlatform(s1) - if o2 == "" && a2 == "" { + s2 := &metav1.LabelSelector{MatchLabels: map[string]string{"os": "non-existing"}} + env2 := findAnyMatchingPlatform(s1) + if env2.OS == "" && env2.Arch == "" { t.Fatalf("with selector %v, expected os/arch", s2) } - s3 := &v1.LabelSelector{MatchExpressions: []v1.LabelSelectorRequirement{{ + s3 := &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ Key: "os", - Operator: v1.LabelSelectorOpIn, + Operator: metav1.LabelSelectorOpIn, Values: []string{"darwin", "linux"}}}} - o3, a3 := findAnyMatchingPlatform(s3) - if o3 == "" || a3 == "" { + env3 := findAnyMatchingPlatform(s3) + if env3.OS == "" || env3.Arch == "" { t.Fatalf("with selector %v, expected os/arch", s2) } } diff --git a/go.sum b/go.sum index d9202bac..d5220130 100644 --- a/go.sum +++ b/go.sum @@ -120,6 +120,7 @@ gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= k8s.io/apimachinery v0.0.0-20190717022731-0bb8574e0887 h1:JVVkMN2P4a3MNzTkjgCCRwBDAGAENrCsZGLoLtQ5jvI= k8s.io/apimachinery v0.0.0-20190717022731-0bb8574e0887/go.mod h1:sBJWIJZfxLhp7mRsRyuAE/NfKTr3kXGR1iaqg8O0gJo= +k8s.io/apimachinery v0.17.0 h1:xRBnuie9rXcPxUkDizUsGvPf1cnlZCFu210op7J7LJo= k8s.io/client-go v7.0.0+incompatible h1:kiH+Y6hn+pc78QS/mtBfMJAMIIaWevHi++JvOGEEQp4= k8s.io/client-go v7.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= diff --git a/internal/installation/platform.go b/internal/installation/platform.go index 71f8dd17..06928c69 100644 --- a/internal/installation/platform.go +++ b/internal/installation/platform.go @@ -31,14 +31,14 @@ import ( // matches the os/arch of the current machine (can be overridden via KREW_OS // and/or KREW_ARCH). func GetMatchingPlatform(platforms []index.Platform) (index.Platform, bool, error) { - return matchPlatform(platforms, osArch()) + return matchPlatform(platforms, OSArch()) } // matchPlatform returns the first matching platform to given os/arch. -func matchPlatform(platforms []index.Platform, env goOSArch) (index.Platform, bool, error) { +func matchPlatform(platforms []index.Platform, env GoOSArch) (index.Platform, bool, error) { envLabels := labels.Set{ - "os": env.os, - "arch": env.arch, + "os": env.OS, + "arch": env.Arch, } klog.V(2).Infof("Matching platform for labels(%v)", envLabels) @@ -55,21 +55,22 @@ func matchPlatform(platforms []index.Platform, env goOSArch) (index.Platform, bo return index.Platform{}, false, nil } -type goOSArch struct { - os, arch string +// GoOSArch is wrapper around operating system and architecture +type GoOSArch struct { + OS, Arch string } // String converts environment into a string -func (env goOSArch) String() string { - return fmt.Sprintf("(%s/%s)", env.os, env.arch) +func (env GoOSArch) String() string { + return fmt.Sprintf("%s/%s", env.OS, env.Arch) } -// osArch returns the OS/arch combination to be used on the current system. It +// OSArch returns the OS/arch combination to be used on the current system. It // can be overridden by setting KREW_OS and/or KREW_ARCH environment variables. -func osArch() goOSArch { - return goOSArch{ - os: getEnvOrDefault("KREW_OS", runtime.GOOS), - arch: getEnvOrDefault("KREW_ARCH", runtime.GOARCH), +func OSArch() GoOSArch { + return GoOSArch{ + OS: getEnvOrDefault("KREW_OS", runtime.GOOS), + Arch: getEnvOrDefault("KREW_ARCH", runtime.GOARCH), } } diff --git a/internal/installation/platform_test.go b/internal/installation/platform_test.go index 8b9aed53..7a554fce 100644 --- a/internal/installation/platform_test.go +++ b/internal/installation/platform_test.go @@ -27,12 +27,12 @@ import ( func Test_osArch(t *testing.T) { inOS, inArch := runtime.GOOS, runtime.GOARCH - out := osArch() - if inOS != out.os { - t.Errorf("returned OS=%q; expected=%q", out.os, inOS) + out := OSArch() + if inOS != out.OS { + t.Errorf("returned OS=%q; expected=%q", out.OS, inOS) } - if inArch != out.arch { - t.Errorf("returned Arch=%q; expected=%q", out.arch, inArch) + if inArch != out.Arch { + t.Errorf("returned Arch=%q; expected=%q", out.Arch, inArch) } } @@ -45,20 +45,20 @@ func Test_osArch_override(t *testing.T) { os.Unsetenv("KREW_OS") }() - out := osArch() - if customOS != out.os { - t.Errorf("returned OS=%q; expected=%q", out.os, customOS) + out := OSArch() + if customOS != out.OS { + t.Errorf("returned OS=%q; expected=%q", out.OS, customOS) } - if customArch != out.arch { - t.Errorf("returned Arch=%q; expected=%q", out.arch, customArch) + if customArch != out.Arch { + t.Errorf("returned Arch=%q; expected=%q", out.Arch, customArch) } } func Test_matchPlatform(t *testing.T) { - target := goOSArch{os: "foo", arch: "amd64"} - matchingPlatform := testutil.NewPlatform().WithOSArch(target.os, target.arch).V() - differentOS := testutil.NewPlatform().WithOSArch("other", target.arch).V() - differentArch := testutil.NewPlatform().WithOSArch(target.os, "other").V() + target := GoOSArch{OS: "foo", Arch: "amd64"} + matchingPlatform := testutil.NewPlatform().WithOSArch(target.OS, target.Arch).V() + differentOS := testutil.NewPlatform().WithOSArch("other", target.Arch).V() + differentArch := testutil.NewPlatform().WithOSArch(target.OS, "other").V() p, ok, err := matchPlatform([]index.Platform{differentOS, differentArch, matchingPlatform}, target) if err != nil { diff --git a/internal/installation/upgrade.go b/internal/installation/upgrade.go index e01af4ee..b329a985 100644 --- a/internal/installation/upgrade.go +++ b/internal/installation/upgrade.go @@ -48,8 +48,8 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error { return errors.Wrap(err, "failed trying to find a matching platform in plugin spec") } if !ok { - return errors.Errorf("plugin %q does not offer installation for this platform %s", - plugin.Name, osArch()) + return errors.Errorf("plugin %q does not offer installation for this platform (%s)", + plugin.Name, OSArch()) } newVersion := plugin.Spec.Version From 0bf3d63776299d4d522f5cfe498314512ccf5e5c Mon Sep 17 00:00:00 2001 From: ferhat elmas Date: Fri, 13 Dec 2019 08:11:27 +0100 Subject: [PATCH 3/5] Run go mod tidy --- go.sum | 1 - 1 file changed, 1 deletion(-) diff --git a/go.sum b/go.sum index d5220130..d9202bac 100644 --- a/go.sum +++ b/go.sum @@ -120,7 +120,6 @@ gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= k8s.io/apimachinery v0.0.0-20190717022731-0bb8574e0887 h1:JVVkMN2P4a3MNzTkjgCCRwBDAGAENrCsZGLoLtQ5jvI= k8s.io/apimachinery v0.0.0-20190717022731-0bb8574e0887/go.mod h1:sBJWIJZfxLhp7mRsRyuAE/NfKTr3kXGR1iaqg8O0gJo= -k8s.io/apimachinery v0.17.0 h1:xRBnuie9rXcPxUkDizUsGvPf1cnlZCFu210op7J7LJo= k8s.io/client-go v7.0.0+incompatible h1:kiH+Y6hn+pc78QS/mtBfMJAMIIaWevHi++JvOGEEQp4= k8s.io/client-go v7.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s= k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= From 3bbfda8f1729454886d5cb58ae43e441ab8006d7 Mon Sep 17 00:00:00 2001 From: ferhat elmas Date: Sat, 14 Dec 2019 15:33:28 +0100 Subject: [PATCH 4/5] Use cmp in tests --- internal/installation/platform_test.go | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/internal/installation/platform_test.go b/internal/installation/platform_test.go index 7a554fce..df512195 100644 --- a/internal/installation/platform_test.go +++ b/internal/installation/platform_test.go @@ -26,31 +26,24 @@ import ( ) func Test_osArch(t *testing.T) { - inOS, inArch := runtime.GOOS, runtime.GOARCH - out := OSArch() - if inOS != out.OS { - t.Errorf("returned OS=%q; expected=%q", out.OS, inOS) - } - if inArch != out.Arch { - t.Errorf("returned Arch=%q; expected=%q", out.Arch, inArch) + in := GoOSArch{OS: runtime.GOOS, Arch: runtime.GOARCH} + + if diff := cmp.Diff(in, OSArch()); diff != "" { + t.Errorf("os/arch got a different result:\n%s", diff) } } func Test_osArch_override(t *testing.T) { - customOS, customArch := "dragons", "metav1" - os.Setenv("KREW_OS", customOS) - os.Setenv("KREW_ARCH", customArch) + customGoOSArch := GoOSArch{OS: "dragons", Arch: "metav1"} + os.Setenv("KREW_OS", customGoOSArch.OS) + os.Setenv("KREW_ARCH", customGoOSArch.Arch) defer func() { os.Unsetenv("KREW_ARCH") os.Unsetenv("KREW_OS") }() - out := OSArch() - if customOS != out.OS { - t.Errorf("returned OS=%q; expected=%q", out.OS, customOS) - } - if customArch != out.Arch { - t.Errorf("returned Arch=%q; expected=%q", out.Arch, customArch) + if diff := cmp.Diff(customGoOSArch, OSArch()); diff != "" { + t.Errorf("os/arch override got a different result:\n%s", diff) } } From 4a908b4acfc0e08a7f28d6d1e45bb8d5d11e8e8b Mon Sep 17 00:00:00 2001 From: ferhat elmas Date: Sun, 15 Dec 2019 15:10:46 +0100 Subject: [PATCH 5/5] Rename GoOSArch to OSArchPair --- cmd/validate-krew-manifest/main.go | 10 +++++----- cmd/validate-krew-manifest/main_test.go | 10 +++++----- internal/installation/platform.go | 14 +++++++------- internal/installation/platform_test.go | 6 +++--- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cmd/validate-krew-manifest/main.go b/cmd/validate-krew-manifest/main.go index e60b26c9..513cca10 100644 --- a/cmd/validate-krew-manifest/main.go +++ b/cmd/validate-krew-manifest/main.go @@ -163,7 +163,7 @@ func installPlatformSpec(manifestFile string, p index.Platform) error { } // findAnyMatchingPlatform finds an pair matches to given selector -func findAnyMatchingPlatform(selector *metav1.LabelSelector) installation.GoOSArch { +func findAnyMatchingPlatform(selector *metav1.LabelSelector) installation.OSArchPair { for _, p := range allPlatforms() { if selectorMatchesOSArch(selector, p) { klog.V(4).Infof("%s MATCHED <%s>", selector, p) @@ -171,10 +171,10 @@ func findAnyMatchingPlatform(selector *metav1.LabelSelector) installation.GoOSAr } klog.V(4).Infof("%s didn't match <%s>", selector, p) } - return installation.GoOSArch{} + return installation.OSArchPair{} } -func selectorMatchesOSArch(selector *metav1.LabelSelector, env installation.GoOSArch) bool { +func selectorMatchesOSArch(selector *metav1.LabelSelector, env installation.OSArchPair) bool { sel, err := metav1.LabelSelectorAsSelector(selector) if err != nil { // this should've been caught by validation.ValidatePlatform() earlier @@ -188,9 +188,9 @@ func selectorMatchesOSArch(selector *metav1.LabelSelector, env installation.GoOS } // allPlatforms returns all pairs krew is supported on. -func allPlatforms() []installation.GoOSArch { +func allPlatforms() []installation.OSArchPair { // TODO(ahmetb) find a more authoritative source for this list - return []installation.GoOSArch{ + return []installation.OSArchPair{ {OS: "windows", Arch: "386"}, {OS: "windows", Arch: "amd64"}, {OS: "linux", Arch: "386"}, diff --git a/cmd/validate-krew-manifest/main_test.go b/cmd/validate-krew-manifest/main_test.go index 30040ad4..cffef1ee 100644 --- a/cmd/validate-krew-manifest/main_test.go +++ b/cmd/validate-krew-manifest/main_test.go @@ -112,7 +112,7 @@ func TestValidateManifestFile(t *testing.T) { func Test_selectorMatchesOSArch(t *testing.T) { type args struct { selector *metav1.LabelSelector - env installation.GoOSArch + env installation.OSArchPair } tests := []struct { name string @@ -123,7 +123,7 @@ func Test_selectorMatchesOSArch(t *testing.T) { name: "label - no match", args: args{ selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, - env: installation.GoOSArch{OS: "windows", Arch: "amd64"}, + env: installation.OSArchPair{OS: "windows", Arch: "amd64"}, }, want: false, }, @@ -131,7 +131,7 @@ func Test_selectorMatchesOSArch(t *testing.T) { name: "label - match", args: args{ selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, - env: installation.GoOSArch{OS: "darwin", Arch: "amd64"}, + env: installation.OSArchPair{OS: "darwin", Arch: "amd64"}, }, want: true, }, @@ -143,7 +143,7 @@ func Test_selectorMatchesOSArch(t *testing.T) { Operator: metav1.LabelSelectorOpIn, Values: []string{"darwin", "linux"}, }}}, - env: installation.GoOSArch{OS: "windows", Arch: "amd64"}, + env: installation.OSArchPair{OS: "windows", Arch: "amd64"}, }, want: false, }, @@ -155,7 +155,7 @@ func Test_selectorMatchesOSArch(t *testing.T) { Operator: metav1.LabelSelectorOpIn, Values: []string{"darwin", "linux"}, }}}, - env: installation.GoOSArch{OS: "darwin", Arch: "amd64"}, + env: installation.OSArchPair{OS: "darwin", Arch: "amd64"}, }, want: true, }, diff --git a/internal/installation/platform.go b/internal/installation/platform.go index 06928c69..3d1f9301 100644 --- a/internal/installation/platform.go +++ b/internal/installation/platform.go @@ -35,7 +35,7 @@ func GetMatchingPlatform(platforms []index.Platform) (index.Platform, bool, erro } // matchPlatform returns the first matching platform to given os/arch. -func matchPlatform(platforms []index.Platform, env GoOSArch) (index.Platform, bool, error) { +func matchPlatform(platforms []index.Platform, env OSArchPair) (index.Platform, bool, error) { envLabels := labels.Set{ "os": env.OS, "arch": env.Arch, @@ -55,20 +55,20 @@ func matchPlatform(platforms []index.Platform, env GoOSArch) (index.Platform, bo return index.Platform{}, false, nil } -// GoOSArch is wrapper around operating system and architecture -type GoOSArch struct { +// OSArchPair is wrapper around operating system and architecture +type OSArchPair struct { OS, Arch string } // String converts environment into a string -func (env GoOSArch) String() string { - return fmt.Sprintf("%s/%s", env.OS, env.Arch) +func (p OSArchPair) String() string { + return fmt.Sprintf("%s/%s", p.OS, p.Arch) } // OSArch returns the OS/arch combination to be used on the current system. It // can be overridden by setting KREW_OS and/or KREW_ARCH environment variables. -func OSArch() GoOSArch { - return GoOSArch{ +func OSArch() OSArchPair { + return OSArchPair{ OS: getEnvOrDefault("KREW_OS", runtime.GOOS), Arch: getEnvOrDefault("KREW_ARCH", runtime.GOARCH), } diff --git a/internal/installation/platform_test.go b/internal/installation/platform_test.go index df512195..e5778f7e 100644 --- a/internal/installation/platform_test.go +++ b/internal/installation/platform_test.go @@ -26,7 +26,7 @@ import ( ) func Test_osArch(t *testing.T) { - in := GoOSArch{OS: runtime.GOOS, Arch: runtime.GOARCH} + in := OSArchPair{OS: runtime.GOOS, Arch: runtime.GOARCH} if diff := cmp.Diff(in, OSArch()); diff != "" { t.Errorf("os/arch got a different result:\n%s", diff) @@ -34,7 +34,7 @@ func Test_osArch(t *testing.T) { } func Test_osArch_override(t *testing.T) { - customGoOSArch := GoOSArch{OS: "dragons", Arch: "metav1"} + customGoOSArch := OSArchPair{OS: "dragons", Arch: "metav1"} os.Setenv("KREW_OS", customGoOSArch.OS) os.Setenv("KREW_ARCH", customGoOSArch.Arch) defer func() { @@ -48,7 +48,7 @@ func Test_osArch_override(t *testing.T) { } func Test_matchPlatform(t *testing.T) { - target := GoOSArch{OS: "foo", Arch: "amd64"} + target := OSArchPair{OS: "foo", Arch: "amd64"} matchingPlatform := testutil.NewPlatform().WithOSArch(target.OS, target.Arch).V() differentOS := testutil.NewPlatform().WithOSArch("other", target.Arch).V() differentArch := testutil.NewPlatform().WithOSArch(target.OS, "other").V()