Skip to content

feat: conclude set-icon experiment - #623

Open
srtaalej wants to merge 1 commit into
mainfrom
release-set-icon
Open

feat: conclude set-icon experiment#623
srtaalej wants to merge 1 commit into
mainfrom
release-set-icon

Conversation

@srtaalej

@srtaalej srtaalej commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Removes the set-icon experiment gate, enabling icon upload via apps.icon.set for all app types by default
  • Removes the deprecated apps.hosted.icon fallback path
  • Updates experiment docs

Test plan

  • All existing tests pass
  • Lint clean
  • Manual test: slack run with an icon file present uploads the icon without needing --experiment set-icon
  • Manual test: slack deploy with an icon file uploads via apps.icon.set

semver:patch

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 27.27273% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.27%. Comparing base (9582a26) to head (82cb461).

Files with missing lines Patch % Lines
internal/pkg/apps/install.go 27.27% 7 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
- Coverage   72.28%   72.27%   -0.01%     
==========================================
  Files         238      238              
  Lines       20087    20080       -7     
==========================================
- Hits        14519    14513       -6     
+ Misses       4299     4298       -1     
  Partials     1269     1269

☔ View full report in Codecov by Harness.
📢 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.

@srtaalej
srtaalej marked this pull request as ready for review August 5, 2026 19:26
@srtaalej
srtaalej requested review from a team as code owners August 5, 2026 19:26
@srtaalej srtaalej self-assigned this Aug 5, 2026
@srtaalej srtaalej added enhancement M-T: A feature request for new functionality experiment Experimental feature accessed behind the --experiment flag or toggle semver:minor Use on pull requests to describe the release version increment labels Aug 5, 2026
@srtaalej srtaalej added this to the Next Release milestone Aug 5, 2026
lukegalbraithrussell
lukegalbraithrussell reviewed Aug 5, 2026
View reviewed changes
Comment thread docs/reference/experiments.md

Below is a list of updates related to experiments.

- **August 2026**: Concluded the `set-icon` experiment with full support for icon upload on all app types now enabled by default in the Slack CLI.

@lukegalbraithrussell lukegalbraithrussell Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **August 2026**: Concluded the `set-icon` experiment with full support for icon upload on all app types now enabled by default in the Slack CLI.
- **August 2026**: Concluded the `set-icon` experiment; The Slack CLI now offers full support for icon upload on all app types by default.

just a little easier to read methinks

@zimeg zimeg added the changelog Use on updates to be included in the release notes label Aug 5, 2026
zimeg
zimeg approved these changes Aug 5, 2026
View reviewed changes

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@srtaalej Thanks for taking this to conclusion! I'm leaving a note that we can perhaps remove more artifacts of the experiment before this merges.

Also a note for the changelog that icon paths are now supported might be an exciting reason to upgrade 📚 I tested with Bolt and ROSI apps for both local and deploy instances with expected results found! 📸

Comment thread internal/pkg/apps/install.go

// updateIcon will upload the new icon to the Slack API
func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string, isHosted bool) error {
func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string) error {

@zimeg zimeg Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🪓 suggestion: This function is used once in Install and I think we can now inline the API call to match InstallLocal patterns? I'd like to avoid separate patterns now that the experiment has concluded.

Comment thread internal/pkg/apps/install_test.go
experimentOn: false,
expectSkip: true,
},
"succeeds with IconSet": {},

@zimeg zimeg Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧪 note: This test doesn't seem so useful without mock data included. Another comment suggested deleting the function it's testing and IMHO this is supported here. Otherwise I'd suggest including:

  • mockFilePath
  • mockSetIconResponse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukegalbraithrussell lukegalbraithrussell lukegalbraithrussell left review comments
@zimeg zimeg zimeg approved these changes

Assignees

@srtaalej srtaalej

Labels

changelog Use on updates to be included in the release notes enhancement M-T: A feature request for new functionality experiment Experimental feature accessed behind the --experiment flag or toggle semver:minor 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.

3 participants

@srtaalej @zimeg @lukegalbraithrussell