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.16% 71.18% +0.02%
==========================================
Files 222 222
Lines 18684 18686 +2
==========================================
+ Hits 13296 13302 +6
Misses 4204 4204
+ Partials 1184 1180 -4 ☔ 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 🧪
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