Skip to content

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

Merged
mwbrooks merged 2 commits intomainfrom
mwbrooks-refactor-internal-pkg-version
Mar 23, 2026
Merged

refactor: move internal/pkg/version to internal/version#428
mwbrooks merged 2 commits intomainfrom
mwbrooks-refactor-internal-pkg-version

Conversation

@mwbrooks
Copy link
Member

@mwbrooks mwbrooks commented Mar 21, 2026 •
edited
Loading

Changelog

  • N/A

Summary

This pull request refactors internal/pkg/version to internal/version. The motivation is to reduce the usage of internal/pkg - all of these packages should eventually find a home outside of internal/pkg.

While this is a small change, it does touch our build configuration.

Test Steps

$ lack --version
# → Confirm the correct SHA is in the version: `-g<SHA>`
#   → "v3.15.0-26-gc082ce3"

Requirements

@mwbrooks mwbrooks added this to the Next Release milestone Mar 21, 2026
@mwbrooks mwbrooks self-assigned this Mar 21, 2026
@mwbrooks 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 build M-T: Changes to compilation and CI processes labels Mar 21, 2026
@mwbrooks mwbrooks force-pushed the mwbrooks-refactor-internal-pkg-version branch from 82423b7 to dacfb62 Compare March 21, 2026 04:37
@codecov
Copy link

codecov bot commented Mar 21, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.24%. Comparing base (ad9ddd9) to head (993a6f5).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #428   +/-   ##
=======================================
  Coverage   70.23%   70.24%           
=======================================
  Files         220      220           
  Lines       18446    18446           
=======================================
+ Hits        12956    12957    +1     
- Misses       4316     4317    +1     
+ Partials     1174     1172    -2

☔ 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 mwbrooks force-pushed the mwbrooks-refactor-internal-pkg-version branch from dacfb62 to c082ce3 Compare March 21, 2026 04:41
mwbrooks
mwbrooks commented Mar 21, 2026
View reviewed changes
Copy link
Member Author

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

A few comments for my kind teammates 🥇

internal/version/version.go
Copy link
Member Author

@mwbrooks mwbrooks Mar 21, 2026

Choose a reason for hiding this comment

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

farewell: So long and thanks for all the fish pkg 🐬

internal/shared/clients.go
@@ -95,8 +95,7 @@ func NewClientFactory(options ...func(*ClientFactory)) *ClientFactory {
// TODO: Temporary hack to get around circular dependency in internal/api/client.go since that imports version
Copy link
Member Author

@mwbrooks mwbrooks Mar 21, 2026

Choose a reason for hiding this comment

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

thought: I did a little research and I believe this TODO is was solved by our slackcontext.Version(ctx) improvements.

I don't want to remove this comment in this PR, because it's out-of-scope, but I'll start to work on a follow-up PR that removes this comment and potentially some other cleanup related to this.

Copy link
Member

@zimeg zimeg Mar 21, 2026

Choose a reason for hiding this comment

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

@mwbrooks Do be warned - The versions expected for login methods upstream can be reliant on something related to version prefixes... 👻

I'm optimistic from the slackcontext enhancements as well but wanted to share prior edge known.

Copy link
Member Author

@mwbrooks mwbrooks Mar 23, 2026

Choose a reason for hiding this comment

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

🧠 Oh yes, that's a good point! I've put together PR #429 and I'll circle back to test the login. I could definitely use your help on thinking about edge-cases in it.

@mwbrooks mwbrooks marked this pull request as ready for review March 21, 2026 04:52
@mwbrooks mwbrooks requested a review from a team as a code owner March 21, 2026 04:52
zimeg
zimeg approved these changes Mar 21, 2026
View reviewed changes
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

🔮 @mwbrooks Another fantastic improvement to code health!

Thanks for keeping these changes ongoing. I left one comment from testing but nothing I noticed in these good changes.

internal/shared/clients.go
@@ -95,8 +95,7 @@ func NewClientFactory(options ...func(*ClientFactory)) *ClientFactory {
// TODO: Temporary hack to get around circular dependency in internal/api/client.go since that imports version
Copy link
Member

@zimeg zimeg Mar 21, 2026

Choose a reason for hiding this comment

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

@mwbrooks Do be warned - The versions expected for login methods upstream can be reliant on something related to version prefixes... 👻

I'm optimistic from the slackcontext enhancements as well but wanted to share prior edge known.

cmd/doctor/check.go
Comment on lines -29 to +34
"github.com/slackapi/slack-cli/internal/pkg/version"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/style"
"github.com/slackapi/slack-cli/internal/update"
"github.com/slackapi/slack-cli/internal/version"
Copy link
Member

@zimeg zimeg Mar 21, 2026

Choose a reason for hiding this comment

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

🔭 praise: This is a nice diff to find!

@mwbrooks mwbrooks mentioned this pull request Mar 23, 2026
Merged
2 tasks
@mwbrooks
Copy link
Member Author

mwbrooks commented Mar 23, 2026

@zimeg Thanks for the review! That's a very good point that our login checks the version and we'll need to be careful that we don't break login for existing and legacy Slack CLI version. 🧠

@mwbrooks mwbrooks merged commit 503fa5d into main Mar 23, 2026
8 checks passed
@mwbrooks mwbrooks deleted the mwbrooks-refactor-internal-pkg-version branch March 23, 2026 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zimeg zimeg zimeg approved these changes

Assignees

@mwbrooks mwbrooks

Labels

build M-T: Changes to compilation and CI processes code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Milestone

Next Release

Development

Successfully merging this pull request may close these issues.

2 participants

@mwbrooks @zimeg