fix: route icon upload by app type in updateIcon#506
Conversation
srtaalej
added
the
semver:patch
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
- Coverage 71.17% 71.15% -0.02%
==========================================
Files 222 222
Lines 18678 18680 +2
==========================================
- Hits 13294 13292 -2
- Misses 4201 4205 +4
Partials 1183 1183 ☔ 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.
| _, err = clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath) | ||
| } else { | ||
| } else if isHosted { | ||
| _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) |
There was a problem hiding this comment.
| _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) | |
| // DEPRECATED: Prefer IconSet once the SetIcon experiment concludes | |
| _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) |
🪬 suggestion(non-blocking): The order of conditionals is solid but we can leave breadcrumbs more to save confusion in upcoming changes. Personal preference though!
2 participants
Summary
This PR fixes
updateIconto route icon uploads by app type instead of toggling on the experiment flag aloneTest plan
go test ./internal/pkg/apps/ -run Installpassesapps.hosted.icon-e set-icon: icon uploads viaapps.icon.setRequirements