refactor: remove legacy CLIVersion field and SetVersion() function by mwbrooks · Pull Request #429 · slackapi/slack-cli

mwbrooks · 2026-03-21T05:50:48Z

Changelog

N/A

Summary

This pull request removes the legacy version references from ClientFactory, including the ClientFactory.CLIVersion field and shared.SetVersion() function.

Removes the CLIVersion field from ClientFactory.
Removes the SetVersion() functional from ClientFactory.
Removes stale TODO comment referencing a circular dependency that no longer exists. This was solved by slackcontext.Version(ctx).
Replaces indirect version access (clients.CLIVersion) with direct version.Raw() calls. The clients.CLIVersion was previously set to the raw version value, so the functionality is unchanged.

Context:

The ClientFactory.CLIVersion and shared.SetVersion() were a workaround for a circular dependency between internal/api/client.go and package version. That dependency no longer exists - internal/api/client.go does not import the version package and the API client now reads the CLI version from context with slackcontext.Version(ctx).

Open Questions:

In a previous PR, we had concerns that this may have unexpected impacts around the login:
- refactor: move internal/pkg/version to internal/version #428 (comment)
- Solution is to test the login command in our Test Steps.

Test Steps

1. Test Version:

$ lack version
# → Expect: "Using slack v3.15.0-26-ga164d25"

$ lack --version
# → Expect: "Using slack v3.15.0-26-ga164d25"

2. Test Version Override with Environment Variable:

$ SLACK_TEST_VERSION=v0.0.1 lack version
# → Expect: "Using slack v0.0.1"

3. Test Doctor Command (confirms Config.Version is set):

$ lack doctor
# → Expect:
#     ✔ CLI (this tool for building Slack apps)
#      Version: v3.15.0-26-ga164d25

4. Test Debug Output (confirms version reaches context)

$ lack auth list --verbose --skip-update 2>&1 | grep -i "version"
# → Expect: "cli_version":"3.15.0-26-ga164d25"

5. Test Login (confirms version accepted by API)

$ lack login
# → Login to a new or existing auth
# → Expect: Login successful

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

The CLIVersion field and SetVersion functional option on ClientFactory were a workaround for a circular dependency that no longer exists. The API client reads version from context via slackcontext.Version(ctx), making this indirection unnecessary. Replace with direct version.Raw() calls.

codecov · 2026-03-21T05:55:14Z

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.21%. Comparing base (503fa5d) to head (1483592).

Files with missing lines	Patch %	Lines
main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
- Coverage   70.24%   70.21%   -0.04%     
==========================================
  Files         220      220              
  Lines       18446    18444       -2     
==========================================
- Hits        12957    12950       -7     
- Misses       4317     4318       +1     
- Partials     1172     1176       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

A few comments for those reviewers eager to see the Slack CLI code improve little by little.

mwbrooks · 2026-03-23T02:48:02Z

cmd/root.go


 	// Init clients that use flags
 	clients.Config.APIHostResolved = clients.Auth().ResolveAPIHost(ctx, clients.Config.APIHostFlag, nil)
-	clients.Config.LogstashHostResolved = clients.Auth().ResolveLogstashHost(ctx, clients.Config.APIHostResolved, clients.CLIVersion)


note: clients.CLIVersion was set to version.Raw().

mwbrooks · 2026-03-23T02:48:07Z

cmd/root.go

 	clients.Config.LoadExperiments(ctx, clients.IO.PrintDebug)
 	style.ToggleLipgloss(clients.Config.WithExperimentOn(experiment.Lipgloss))
 	// TODO(slackcontext) Consolidate storing CLI version to slackcontext
-	clients.Config.Version = clients.CLIVersion


note: clients.CLIVersion was set to version.Raw().

mwbrooks · 2026-03-23T02:48:28Z

internal/shared/clients.go

-	// TODO: Temporary hack to get around circular dependency in internal/api/client.go since that imports version
-	// Follows pattern demonstrated by the GitHub CLI here https://github.com/cli/cli/blob/5a46c1cab601a3394caa8de85adb14f909b811e9/pkg/cmd/factory/default.go#L29
-	// Used by the APIClient for its userAgent
-	// Currently needed because trying to get the version of the CLI from pkg/version/version.go would cause a circular dependency
-	// We can get rid of this once we refactor the code relationship between pkg/ and internal/
-	// userAgent can get Slack CLI version from context which is defined in main.go, this approach bypass circular dependency. The clients.CLIVersion is retained for future code refactor purpose and serve SetVersion function
-	clients.CLIVersion = ""
-


🌲 praise: This has bothered me more than I understood. Thanks for making change that improves the code @mwbrooks!

mwbrooks · 2026-03-23T02:50:58Z

main.go

+		var clients = shared.NewClientFactory()
+		clients.Config.Version = version.Raw()


thought: I still don't like that we're setting the version in multiple places, instead of just referencing the source of truth (version.Raw() or version.Get()). However, I think this PR is a step toward simpler code with fewer indirect references. We can follow-up further in more PRs if we want.

🔭 thought: For this panic catching I think it's alright to forgive some duplication and strange logic.

🌟 praise: Thanks for catching this in these updates still!

zimeg

📚 Amazing changes to start the week with! Thanks for untangling some of the more intricate logic here. I left a few notes of optional reading!

zimeg · 2026-03-23T16:17:35Z

main.go

+		var clients = shared.NewClientFactory()
+		clients.Config.Version = version.Raw()


🔭 thought: For this panic catching I think it's alright to forgive some duplication and strange logic.

🌟 praise: Thanks for catching this in these updates still!

zimeg · 2026-03-23T16:18:53Z

cmd/root.go

 	style.ToggleLipgloss(clients.Config.WithExperimentOn(experiment.Lipgloss))
 	// TODO(slackcontext) Consolidate storing CLI version to slackcontext
-	clients.Config.Version = clients.CLIVersion
+	clients.Config.Version = version.Raw()


🔍 thought(non-blocking): Would setting this version above - before L268 above - let us reuse the configuration version instead of calling to the version package twice?

👾 ramble: I'm unsure if this value is set here due to dependencies above, but I might expect the version to remain stable during execution here!

🗣️ ramble: Forgive me if the TODO of "slackcontext" above is meant to capture similar changes!

zimeg · 2026-03-23T16:25:04Z

internal/shared/clients.go

-	// TODO: Temporary hack to get around circular dependency in internal/api/client.go since that imports version
-	// Follows pattern demonstrated by the GitHub CLI here https://github.com/cli/cli/blob/5a46c1cab601a3394caa8de85adb14f909b811e9/pkg/cmd/factory/default.go#L29
-	// Used by the APIClient for its userAgent
-	// Currently needed because trying to get the version of the CLI from pkg/version/version.go would cause a circular dependency
-	// We can get rid of this once we refactor the code relationship between pkg/ and internal/
-	// userAgent can get Slack CLI version from context which is defined in main.go, this approach bypass circular dependency. The clients.CLIVersion is retained for future code refactor purpose and serve SetVersion function
-	clients.CLIVersion = ""
-


🌲 praise: This has bothered me more than I understood. Thanks for making change that improves the code @mwbrooks!

mwbrooks added this to the Next Release milestone Mar 21, 2026

mwbrooks self-assigned this Mar 21, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 21, 2026

Merge branch 'main' into mwbrooks-cleanup-cli-version

24e129b

mwbrooks commented Mar 23, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 23, 2026 02:51

mwbrooks requested a review from a team as a code owner March 23, 2026 02:51

mwbrooks changed the title ~~refactor: remove legacy CLIVersion field and SetVersion option~~ refactor: remove legacy CLIVersion field and SetVersion() function Mar 23, 2026

mwbrooks mentioned this pull request Mar 23, 2026

refactor: move internal/pkg/version to internal/version #428

Merged

2 tasks

Merge branch 'main' into mwbrooks-cleanup-cli-version

1483592

zimeg approved these changes Mar 23, 2026

View reviewed changes

		var clients = shared.NewClientFactory()
		clients.Config.Version = version.Raw()

Conversation

mwbrooks commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Test Steps

Requirements

Uh oh!

codecov bot commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Mar 21, 2026 •

edited

Loading

codecov bot commented Mar 21, 2026 •

edited

Loading