fix: avoid icon upload without experiment for non-hosted apps#506
fix: avoid icon upload without experiment for non-hosted apps#506
Conversation
srtaalej
added
the
semver:patch
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
+ Coverage 71.18% 71.24% +0.06%
==========================================
Files 222 222
Lines 18680 18682 +2
==========================================
+ Hits 13297 13310 +13
+ Misses 4202 4189 -13
- Partials 1181 1183 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if clients.Config.WithExperimentOn(experiment.SetIcon) { | ||
| if isHosted { | ||
| _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) | ||
| } else if clients.Config.WithExperimentOn(experiment.SetIcon) { | ||
| _, err = clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath) | ||
| } else { | ||
| _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🪬 question: Are we hoping to have all uploads use apps.icon.set? I find this works as expected for hosted apps at the moment and am unsure that we need to change this.
There was a problem hiding this comment.
hmm agreed - i guess i was unsure whether or not we were completely dropping apps.hosted.icon support 🤔 should the experiment only work with apps.icon.set then?
There was a problem hiding this comment.
@srtaalej I understand apps.icon.set to be the replacement and preferred method for updating an app icon now! Please correct me if this is wrong, but we should use that while the experiment is active if so 🧪
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Thanks for these updates! I understand the fix this PR is addressing now and changes LGTM 🚢
Before merging I'm wondering if some PR formalities can be polished? The confusion I had most was in the intent of this PR. The title explains what the code does but not why this change was needed. I might suggest:
fix: avoid icon upload without experiment for non-hosted apps
Including a note in the description of when this might happen can be helpful in sharing about the issue as this is adjusting behavior in a current release. Perhaps with a changelog too?
The test plan also did not give me much to start with. Sharing the commands you used to test the development build with can make review faster for me if I can ask for that 🙏
Something brief gives a lot of focus and we can save unit test steps for CI:
$ slack run -e set-icon # Confirm icon uploads for Bolt appWe should also add a bug label and milestone to this before we merge so our release notes find it in the right spot.
srtaalej
added
the
bug
Co-Authored-By: Claude <[email protected]>
srtaalej
changed the title
2 participants
Summary
set-iconexperiment enabled, non-hosted (Bolt) apps were still attempting to upload an icon viaapps.hosted.icon, which is only valid for hosted appsapps.icon.set(works for all app types), experiment off + hosted →apps.hosted.icon, experiment off + non-hosted → skipDEPRECATEDcomment on theapps.hosted.iconpath per reviewer suggestionTest plan
go test ./internal/pkg/apps/ -run Test_updateIcon— 6 cases covering all routing paths and error handlingRequirements