Skip to content

feat: add debug logging and retry support to icon upload#505

Open
srtaalej wants to merge 2 commits intomainfrom
ale-upload-icon
Open

feat: add debug logging and retry support to icon upload#505
srtaalej wants to merge 2 commits intomainfrom
ale-upload-icon

Conversation

@srtaalej
Copy link
Copy Markdown
Contributor

Changelog

Add verbose debug logging and retry support to icon upload API calls.

Summary

This PR refactors uploadIcon to use DoWithRetry instead of calling httpClient.Do directly, matching the pattern used by all other API methods. It adds verbose debug logging for icon upload requests and responses and automatic retry on rate limits for icon uploads

Addresses review feedback from #469 (comment)

Test plan

  • go test ./internal/api/ -run Icon passes
  • make build then slack run -e set-icon --verbose from a project with icon.png — confirm
    HTTP request/response details appear in verbose logs without binary noise

Requirements

@srtaalej srtaalej added this to the Next Release milestone Apr 20, 2026
@srtaalej srtaalej self-assigned this Apr 20, 2026
@srtaalej srtaalej requested a review from a team as a code owner April 20, 2026 15:26
@srtaalej srtaalej added the semver:patch Use on pull requests to describe the release version increment label Apr 20, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.16%. Comparing base (5ffec6b) to head (560402d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
- Coverage   71.17%   71.16%   -0.02%     
==========================================
  Files         222      222              
  Lines       18684    18680       -4     
==========================================
- Hits        13299    13294       -5     
  Misses       4203     4203              
- Partials     1182     1183       +1     

☔ 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.

@zimeg zimeg added the enhancement M-T: A feature request for new functionality label Apr 20, 2026
Copy link
Copy Markdown
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.

@srtaalej Super nice improvements to help troubleshooting! 📫

I'm hoping we can land a change or two more before this merges. One is in comments below but also I think we can remove these logs from output now too:

clients.IO.PrintDebug(ctx, "uploading icon")

clients.IO.PrintDebug(ctx, "uploading icon")

Comment thread internal/api/icon.go
return IconResult{}, err
}
defer resp.Body.Close()
c.io.PrintDebug(ctx, "HTTP Request: %v %v", request.Method, request.URL)
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.

Suggested change
c.io.PrintDebug(ctx, "HTTP Request: %v %v", request.Method, request.URL)
c.io.PrintDebug(ctx, "HTTP Request: %v %v %v", request.Method, request.URL, request.Proto)

🔭 suggestion: To match the entire request signature:

HTTP Request: POST https://slack.com/api/apps.icon.set HTTP/1.1

@zimeg zimeg added the changelog Use on updates to be included in the release notes label Apr 20, 2026
@zimeg zimeg changed the title fix: add debug logging and retry support to icon upload feat: add debug logging and retry support to icon upload Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog Use on updates to be included in the release notes enhancement M-T: A feature request for new functionality semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants