Skip to content

refactor: remove 'version.Get()' and only use 'version.Raw()' #445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwbrooks merged 3 commits into from
Mar 30, 2026
Merged

refactor: remove 'version.Get()' and only use 'version.Raw()' #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e114959
refactor: remove version.Get() and use only version.Raw()
mwbrooks Mar 26, 2026
218fa43
refactor: use strings.HasPrefix instead of regexp in ensurePrefix
mwbrooks Mar 30, 2026
bc1d164
Merge branch 'main' into mwbrooks-remove-version-get
mwbrooks Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/doctor/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import (
)

func TestDoctorCommand(t *testing.T) {
expectedCLIVersion := version.Get()
expectedCLIVersion := version.Raw()
expectedCredentials := types.SlackAuth{
TeamDomain: "team123",
TeamID: "T123",
Expand Down
6 changes: 3 additions & 3 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,14 @@ func NewRootCommand(clients *shared.ClientFactory, updateNotification *update.Up
})

// Check for an CLI update in the background while the command runs
updateNotification = update.New(clients, version.Get(), "SLACK_SKIP_UPDATE")
updateNotification = update.New(clients, version.Raw(), "SLACK_SKIP_UPDATE")
updateNotification.CheckForUpdateInBackground(ctx, false)
return nil
},
PersistentPostRunE: func(cmd *cobra.Command, args []string) error {
// TODO: since commands are moving to `*E` cobra lifecycle methods, this method may not be invoked if those earlier lifecycle methods return an error. Maybe move this to the cleanup() method below? but maybe this is OK, no need to prompt users to update if they encounter an error?
// when the command is `slack update`
return updateNotification.PrintAndPromptUpdates(cmd, version.Get())
return updateNotification.PrintAndPromptUpdates(cmd, version.Raw())
},
RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
Expand All @@ -153,7 +153,7 @@ func Init(ctx context.Context) (*cobra.Command, *shared.ClientFactory) {

// Support `--version` by setting root command's `Version` and custom template.
// Add a newline to `SetVersionTemplate` to display correctly on terminals.
rootCmd.Version = version.Get()
rootCmd.Version = version.Raw()
rootCmd.SetVersionTemplate(versioncmd.Template() + "\n")

// Add subcommands (each subcommand may add their own child subcommands)
Expand Down
2 changes: 1 addition & 1 deletion cmd/upgrade/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func NewCommand(clients *shared.ClientFactory) *cobra.Command {
// When there are updates, the function will *not* print a message because the root command handles printing update notifications.
func checkForUpdates(clients *shared.ClientFactory, cmd *cobra.Command) error {
ctx := cmd.Context()
updateNotification := update.New(clients, version.Get(), "SLACK_SKIP_UPDATE")
updateNotification := update.New(clients, version.Raw(), "SLACK_SKIP_UPDATE")
Comment on lines 57 to +58
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎁 suggestion(non-blocking): In later changes to upgrade do we want to prefer slackcontext packages instead of importing version here?

slack-cli/internal/slackcontext/slackcontext.go

Lines 174 to 184 in e114959

// Version returns the CLI version associated with `ctx`. If the version is not
// found in the context, it falls back to version.Raw() and returns an error to
// signal that the context was not properly initialized.
func Version(ctx context.Context) (string, error) {
var v, ok = ctx.Value(contextKeyVersion).(string)
if !ok || v == "" {
return version.Raw(), slackerror.New(slackerror.ErrContextValueNotFound).
WithMessage("The value for version could not be found in context, falling back to the build version")
}
return v, nil
}

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. During this PR, I was surprised to see so many version.Raw() calls still remain. In the follow-up PR, I'll aim to replace these - it's less dangerous now that slackcontext.Version(ctx) fallsback on the raw version. 👍🏻


// TODO(@mbrooks) This update check is happening at the same time as the root command's `CheckForUpdateInBackground`.
// The difference between the two is that this update check is forced while the root command runs every 24 hours.
Expand Down
2 changes: 1 addition & 1 deletion cmd/version/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func NewCommand(clients *shared.ClientFactory) *cobra.Command {

func Template() string {
processName := cmdutil.GetProcessName()
version := version.Get()
version := version.Raw()

return fmt.Sprintf(style.Secondary("Using %s %s"), processName, version)
}
6 changes: 3 additions & 3 deletions internal/pkg/auth/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func createNewAuth(ctx context.Context, apiClient api.APIInterface, authClient a
return types.SlackAuth{}, "", err
}

authExchangeRes, err := apiClient.ExchangeAuthTicket(ctx, authTicket, challengeCode, version.Get())
authExchangeRes, err := apiClient.ExchangeAuthTicket(ctx, authTicket, challengeCode, version.Raw())
if err != nil {
return types.SlackAuth{}, "", err
}
Expand All @@ -162,7 +162,7 @@ func requestAuthTicket(ctx context.Context, apiClient api.APIInterface, io iostr
span, ctx = opentracing.StartSpanFromContext(ctx, "requestAuthTicket")
defer span.Finish()

cliVersion := semver.MajorMinor(version.Get())
cliVersion := semver.MajorMinor(version.Raw())

// Get a ticket from Slack
if authTicketResult, err := apiClient.GenerateAuthTicket(ctx, cliVersion, noRotation); err != nil {
Expand Down Expand Up @@ -246,7 +246,7 @@ func LoginNoPrompt(ctx context.Context, clients *shared.ClientFactory, ticketArg

// existing ticket request, try to exchange
if ticketArg != "" && challengeCodeArg != "" {
authExchangeRes, err := clients.API().ExchangeAuthTicket(ctx, ticketArg, challengeCodeArg, version.Get())
authExchangeRes, err := clients.API().ExchangeAuthTicket(ctx, ticketArg, challengeCodeArg, version.Raw())
if err != nil || !authExchangeRes.IsReady {
return types.SlackAuth{}, "", err
}
Expand Down
16 changes: 8 additions & 8 deletions internal/version/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package version

import (
"os"
"regexp"
"strings"
)

Expand All @@ -33,23 +32,24 @@ func init() {
if envVersion := getVersionFromEnv(); envVersion != "" {
Version = envVersion
}
Version = ensurePrefix(Version)
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: We now enforce the v prefix on init() so that all version-pathways will now have a v prefix.

Copy link
Copy Markdown
Member

@zimeg zimeg Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦠 thought: I like this approach! It keeps the ctx corrent and commands are well structured for this I believe!

}

// getVersionFromEnv will return the formatted version from EnvTestVersion otherwise "".
func getVersionFromEnv() string {
return strings.Trim(os.Getenv(EnvTestVersion), " ")
}

// Get the version and format it (e.g. `v1.0.0`)
func Get() string {
version := Version
if match, _ := regexp.MatchString(`^[^v]`, version); match {
version = "v" + version
// ensurePrefix ensures that the version string has a "v" prefix.
// Empty strings are returned as-is to avoid producing a bare "v".
func ensurePrefix(v string) string {
if v != "" && !strings.HasPrefix(v, "v") {
return "v" + v
}
return version
return v
}

// Raw returns the raw, unformatted version
// Raw returns the version
func Raw() string {
return Version
}
8 changes: 2 additions & 6 deletions internal/version/version_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ func TestVersion(t *testing.T) {
assert.True(t, len(v) > 0, "some default value exists")
}

// Test overriding the Version with an environment variable
func Test_Get(t *testing.T) {
func Test_ensurePrefix(t *testing.T) {
tests := map[string]struct {
version string
expected string
Expand All @@ -52,10 +51,7 @@ func Test_Get(t *testing.T) {
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
original := Version
defer func() { Version = original }()
Version = tc.version
assert.Equal(t, tc.expected, Get())
Comment on lines -55 to -58
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪓 praise: Huge improvement IMHO!

assert.Equal(t, tc.expected, ensurePrefix(tc.version))
})
}
}
Expand Down
Loading