From 1ca0e900b21f5ff58e29e344f6ca4275a9953d4d Mon Sep 17 00:00:00 2001 From: Gianluca Stella Date: Mon, 11 May 2026 11:55:00 +0200 Subject: [PATCH 1/2] Disable self-upgrade and version check for package-manager installs When osdctl is installed via COPR or Homebrew, the self-upgrade command would overwrite a file owned by the package manager and the version-check nag is unhelpful. This adds a build-time InstallMethod variable (set via ldflags) that disables both behaviors and directs users to their package manager instead. Co-Authored-By: Claude Opus 4.6 --- cmd/cmd.go | 6 ++--- cmd/upgrade.go | 7 ++++++ cmd/upgrade_test.go | 43 +++++++++++++++++++++++++++++++++++ cmd/version.go | 14 +++++++----- hack/copr.sh | 2 +- hack/osdctl.spec | 2 +- pkg/utils/version.go | 25 ++++++++++++++++++++ pkg/utils/version_test.go | 48 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 cmd/upgrade_test.go create mode 100644 pkg/utils/version_test.go diff --git a/cmd/cmd.go b/cmd/cmd.go index af84635ea..49a4d479e 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -124,9 +124,9 @@ func NewCmdRoot(streams genericclioptions.IOStreams) *cobra.Command { // Checks if the version check should be run func shouldRunVersionCheck(skipVersionCheckFlag bool, commandName string) bool { - - // If either are true, then the version check should NOT run, hence negation - return !(skipVersionCheckFlag || canCommandSkipVersionCheck(commandName)) + return !skipVersionCheckFlag && + !canCommandSkipVersionCheck(commandName) && + !utils.IsManagedInstall() } func canCommandSkipVersionCheck(commandName string) bool { diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 1993590ac..8a5ced0de 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -26,6 +26,13 @@ var upgradeCmd = &cobra.Command{ } func upgrade(cmd *cobra.Command, args []string) error { + if utils.IsManagedInstall() { + fmt.Fprintf(cmd.ErrOrStderr(), + "osdctl was installed via %s; self-upgrade is disabled.\nPlease upgrade using: %s\n", + utils.InstallMethod, utils.UpgradeInstruction()) + return nil + } + // rootName ensures that the upgrade will fail if we ever decide to rename osdctl // between releases :-) rootName := cmd.Root().Name() diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go new file mode 100644 index 000000000..9d9f57e47 --- /dev/null +++ b/cmd/upgrade_test.go @@ -0,0 +1,43 @@ +package cmd + +import ( + "bytes" + "strings" + "testing" + + "github.com/openshift/osdctl/pkg/utils" +) + +func TestUpgradeRefusesWhenManaged(t *testing.T) { + tests := []struct { + name string + installMethod string + wantSubstring string + }{ + {"copr", "copr", "dnf upgrade osdctl"}, + {"homebrew", "homebrew", "brew upgrade osdctl"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := utils.InstallMethod + defer func() { utils.InstallMethod = original }() + utils.InstallMethod = tt.installMethod + + var buf bytes.Buffer + upgradeCmd.SetErr(&buf) + defer upgradeCmd.SetErr(nil) + + err := upgrade(upgradeCmd, nil) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + output := buf.String() + if !strings.Contains(output, tt.installMethod) { + t.Errorf("output should mention %q, got: %s", tt.installMethod, output) + } + if !strings.Contains(output, tt.wantSubstring) { + t.Errorf("output should contain %q, got: %s", tt.wantSubstring, output) + } + }) + } +} diff --git a/cmd/version.go b/cmd/version.go index b23577375..824b71ce4 100644 --- a/cmd/version.go +++ b/cmd/version.go @@ -13,9 +13,10 @@ import ( // versionResponse is necessary for the JSON version response. It uses the three // variables that get set during the build. type versionResponse struct { - Commit string `json:"commit"` - Version string `json:"version"` - Latest string `json:"latest"` + Commit string `json:"commit"` + Version string `json:"version"` + Latest string `json:"latest"` + InstallMethod string `json:"install_method,omitempty"` } // versionCmd is the subcommand "osdctl version" for cobra. @@ -41,9 +42,10 @@ func version(cmd *cobra.Command, args []string) error { latest, _ := utils.GetLatestVersion() // let's ignore this error, just in case we have no internet access ver, err := json.MarshalIndent(&versionResponse{ - Commit: gitCommit, - Version: utils.Version, - Latest: strings.TrimPrefix(latest, "v"), + Commit: gitCommit, + Version: utils.Version, + Latest: strings.TrimPrefix(latest, "v"), + InstallMethod: utils.InstallMethod, }, "", " ") if err != nil { return err diff --git a/hack/copr.sh b/hack/copr.sh index 84c1e8bb8..2af9c29b8 100644 --- a/hack/copr.sh +++ b/hack/copr.sh @@ -83,7 +83,7 @@ Source: %{gosource} %if %{without bootstrap} %define gomodulesmode GO111MODULE=on %build -export GO_LDFLAGS='-X "github.com/openshift/osdctl/pkg/utils.Version="@version@"' +export GO_LDFLAGS='-X "github.com/openshift/osdctl/pkg/utils.Version=@version@" -X "github.com/openshift/osdctl/pkg/utils.InstallMethod=copr"' %gobuild -o %{gobuilddir}/bin/osdctl %{goipath} %endif diff --git a/hack/osdctl.spec b/hack/osdctl.spec index 44bb149db..df47e535d 100644 --- a/hack/osdctl.spec +++ b/hack/osdctl.spec @@ -56,7 +56,7 @@ Source: %{gosource} %if %{without bootstrap} %define gomodulesmode GO111MODULE=on %build -export GO_LDFLAGS='-X "github.com/openshift/osdctl/pkg/utils.Version="@version@"' +export GO_LDFLAGS='-X "github.com/openshift/osdctl/pkg/utils.Version=@version@" -X "github.com/openshift/osdctl/pkg/utils.InstallMethod=copr"' %gobuild -o %{gobuilddir}/bin/osdctl %{goipath} %endif diff --git a/pkg/utils/version.go b/pkg/utils/version.go index 5ce13b32f..692e5828d 100644 --- a/pkg/utils/version.go +++ b/pkg/utils/version.go @@ -22,8 +22,33 @@ var ( // Will be set during build process via GoReleaser // See also: https://pkg.go.dev/cmd/link Version string + + // InstallMethod is set at build time via -X ldflags when osdctl is + // built by a package manager. Empty string (default) means the binary + // was built from source or via GoReleaser (GitHub releases). + // Known values: "copr", "homebrew". + InstallMethod string ) +// IsManagedInstall reports whether osdctl was installed via a package +// manager (e.g. COPR/RPM, Homebrew) rather than from a GitHub release. +func IsManagedInstall() bool { + return InstallMethod != "" +} + +// UpgradeInstruction returns a human-readable upgrade command for the +// current install method. Returns empty string for non-managed installs. +func UpgradeInstruction() string { + switch InstallMethod { + case "copr": + return "dnf upgrade osdctl" + case "homebrew": + return "brew upgrade osdctl" + default: + return "" + } +} + // githubResponse is a necessary struct for the JSON unmarshalling that is happening // in the getLatestVersion(). type gitHubResponse struct { diff --git a/pkg/utils/version_test.go b/pkg/utils/version_test.go new file mode 100644 index 000000000..65e273861 --- /dev/null +++ b/pkg/utils/version_test.go @@ -0,0 +1,48 @@ +package utils + +import "testing" + +func TestIsManagedInstall(t *testing.T) { + tests := []struct { + name string + installMethod string + want bool + }{ + {"empty (GitHub release)", "", false}, + {"copr", "copr", true}, + {"homebrew", "homebrew", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := InstallMethod + defer func() { InstallMethod = original }() + InstallMethod = tt.installMethod + if got := IsManagedInstall(); got != tt.want { + t.Errorf("IsManagedInstall() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestUpgradeInstruction(t *testing.T) { + tests := []struct { + name string + installMethod string + want string + }{ + {"copr", "copr", "dnf upgrade osdctl"}, + {"homebrew", "homebrew", "brew upgrade osdctl"}, + {"empty", "", ""}, + {"unknown", "unknown", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := InstallMethod + defer func() { InstallMethod = original }() + InstallMethod = tt.installMethod + if got := UpgradeInstruction(); got != tt.want { + t.Errorf("UpgradeInstruction() = %q, want %q", got, tt.want) + } + }) + } +} From aaa7250c2af22dd37edc8a9db3cef8ff6cca5880 Mon Sep 17 00:00:00 2001 From: Gianluca Stella Date: Mon, 11 May 2026 12:19:15 +0200 Subject: [PATCH 2/2] Error on unknown install method in upgrade command UpgradeInstruction() now returns an error for unrecognized InstallMethod values, surfacing build misconfigurations instead of printing an empty upgrade instruction. Co-Authored-By: Claude Opus 4.6 --- cmd/upgrade.go | 6 +++++- cmd/upgrade_test.go | 15 +++++++++++++-- pkg/utils/version.go | 13 ++++++++----- pkg/utils/version_test.go | 16 +++++++++++----- 4 files changed, 37 insertions(+), 13 deletions(-) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 8a5ced0de..133c95327 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -27,9 +27,13 @@ var upgradeCmd = &cobra.Command{ func upgrade(cmd *cobra.Command, args []string) error { if utils.IsManagedInstall() { + instruction, err := utils.UpgradeInstruction() + if err != nil { + return err + } fmt.Fprintf(cmd.ErrOrStderr(), "osdctl was installed via %s; self-upgrade is disabled.\nPlease upgrade using: %s\n", - utils.InstallMethod, utils.UpgradeInstruction()) + utils.InstallMethod, instruction) return nil } diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 9d9f57e47..80c415037 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -13,9 +13,11 @@ func TestUpgradeRefusesWhenManaged(t *testing.T) { name string installMethod string wantSubstring string + wantErr bool }{ - {"copr", "copr", "dnf upgrade osdctl"}, - {"homebrew", "homebrew", "brew upgrade osdctl"}, + {"copr", "copr", "dnf upgrade osdctl", false}, + {"homebrew", "homebrew", "brew upgrade osdctl", false}, + {"unknown", "unknown", "unknown install method", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -28,6 +30,15 @@ func TestUpgradeRefusesWhenManaged(t *testing.T) { defer upgradeCmd.SetErr(nil) err := upgrade(upgradeCmd, nil) + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.wantSubstring) { + t.Errorf("error should contain %q, got: %s", tt.wantSubstring, err.Error()) + } + return + } if err != nil { t.Fatalf("expected nil error, got: %v", err) } diff --git a/pkg/utils/version.go b/pkg/utils/version.go index 692e5828d..120ec33c0 100644 --- a/pkg/utils/version.go +++ b/pkg/utils/version.go @@ -2,6 +2,7 @@ package utils import ( "encoding/json" + "fmt" "io" "net/http" "time" @@ -37,15 +38,17 @@ func IsManagedInstall() bool { } // UpgradeInstruction returns a human-readable upgrade command for the -// current install method. Returns empty string for non-managed installs. -func UpgradeInstruction() string { +// current install method. +func UpgradeInstruction() (string, error) { switch InstallMethod { + case "": + return "", nil case "copr": - return "dnf upgrade osdctl" + return "dnf upgrade osdctl", nil case "homebrew": - return "brew upgrade osdctl" + return "brew upgrade osdctl", nil default: - return "" + return "", fmt.Errorf("unknown install method: %q", InstallMethod) } } diff --git a/pkg/utils/version_test.go b/pkg/utils/version_test.go index 65e273861..50c75a971 100644 --- a/pkg/utils/version_test.go +++ b/pkg/utils/version_test.go @@ -29,18 +29,24 @@ func TestUpgradeInstruction(t *testing.T) { name string installMethod string want string + wantErr bool }{ - {"copr", "copr", "dnf upgrade osdctl"}, - {"homebrew", "homebrew", "brew upgrade osdctl"}, - {"empty", "", ""}, - {"unknown", "unknown", ""}, + {"copr", "copr", "dnf upgrade osdctl", false}, + {"homebrew", "homebrew", "brew upgrade osdctl", false}, + {"empty", "", "", false}, + {"unknown", "unknown", "", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { original := InstallMethod defer func() { InstallMethod = original }() InstallMethod = tt.installMethod - if got := UpgradeInstruction(); got != tt.want { + got, err := UpgradeInstruction() + if (err != nil) != tt.wantErr { + t.Errorf("UpgradeInstruction() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { t.Errorf("UpgradeInstruction() = %q, want %q", got, tt.want) } })