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..513cca10 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.OSArchPair { 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.OSArchPair{} } -func selectorMatchesOSArch(selector *metav1.LabelSelector, os, arch string) 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 @@ -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.OSArchPair { // 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.OSArchPair{ + {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..cffef1ee 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.OSArchPair } 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.OSArchPair{OS: "windows", Arch: "amd64"}, + }, + want: false, + }, + { + name: "label - match", + args: args{ + selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}, + env: installation.OSArchPair{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.OSArchPair{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.OSArchPair{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/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..3d1f9301 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 OSArchPair) (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,29 @@ func matchPlatform(platforms []index.Platform, os, arch string) (index.Platform, return index.Platform{}, false, nil } -// osArch returns the OS/arch combination to be used on the current system. It +// OSArchPair is wrapper around operating system and architecture +type OSArchPair struct { + OS, Arch string +} + +// String converts environment into a string +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() (string, string) { - goos, goarch := runtime.GOOS, runtime.GOARCH - envOS, envArch := os.Getenv("KREW_OS"), os.Getenv("KREW_ARCH") - if envOS != "" { - goos = envOS +func OSArch() OSArchPair { + return OSArchPair{ + 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..e5778f7e 100644 --- a/internal/installation/platform_test.go +++ b/internal/installation/platform_test.go @@ -26,41 +26,34 @@ 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) - } - if inArch != outArch { - t.Errorf("returned Arch=%q; expected=%q", outArch, inArch) + 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) } } func Test_osArch_override(t *testing.T) { - customOS, customArch := "dragons", "metav1" - os.Setenv("KREW_OS", customOS) - os.Setenv("KREW_ARCH", customArch) + customGoOSArch := OSArchPair{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") }() - outOS, outArch := osArch() - if customOS != outOS { - t.Errorf("returned OS=%q; expected=%q", outOS, customOS) - } - if customArch != outArch { - t.Errorf("returned Arch=%q; expected=%q", outArch, customArch) + if diff := cmp.Diff(customGoOSArch, OSArch()); diff != "" { + t.Errorf("os/arch override got a different result:\n%s", diff) } } 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 := 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() - 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 +64,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..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 { - 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"