autopprof icon indicating copy to clipboard operation
autopprof copied to clipboard

chore: Use UploadFileV2Context due to files.upload API deprecation

Open mung9 opened this issue 1 year ago • 6 comments

Background

SlackReporter currently uploads files to Slack channels using the UploadFileContext function from slack-go/slack. This function internally calls the files.upload web API, but this API has been deprecated, leading to two key implications:

  • From May 8, 2024, newly created Slack apps will no longer be able to use this API.
  • From March 11, 2025, all Slack apps will be unable to use this API.

This PR updates the file upload method to support new Slack apps and addresses the upcoming deprecation.

The official documentation explains the deprecation as follows:

files.upload is deprecated and will stop functioning on March 11, 2025. Use files.getUploadURLExternal and files.completeUploadExternal for file uploads instead. Newly created apps will be unable to use files.upload starting May 8, 2024. See Uploading files for more details on the process, and check out this changelog for more on the deprecation.

Changes

1. Updated to use UploadFileV2Context instead of UploadFileContext.

The function has been updated to call UploadFileV2Context, which implements the latest file upload method. Since the old version didn’t support this, the slack-go/slack version was upgraded to 0.14.0 (Latest). This version supports Go 1.16, so there are no compatibility issues with the Go language version.

The v2 function differs from the old version in two significant ways:

  1. The v2 function requires the Slack channel’s ID to upload a file. You can no longer pass the channel name.
  2. The v2 function requires the size of the file being uploaded to be specified.

These differences required some additional changes:

2. Modified the SlackReporter constructor to accept a Channel ID.

Previously, when creating a SlackReporter, you could pass either a channel name or ID. Now, the Channel parameter has been deprecated, and a ChannelID parameter has been added instead.

3. Updated the ReportXXProfile functions in the Reporter interface to accept file size as a parameter.

Previously, to upload profile information, you only needed to pass an io.Reader. Now, you must also specify the file size.

Migration guide

Finding the Channel ID

You can find the channel ID by right-clicking the channel name and opening the details page.

image

Urgency

I hope to have the review completed by October 11th.

mung9 avatar Sep 24 '24 07:09 mung9

I’m trying to add @mingrammer as a reviewer, but they are not showing up in the reviewer search. Could you please check and add them if you can find them? Thank you!

mung9 avatar Sep 25 '24 13:09 mung9

hello @mung9. Is it possible to rewrite the description by English? This repo is public, so that would be nice.

MeteorSis avatar Sep 26 '24 01:09 MeteorSis

hello @mung9. Is it possible to rewrite the description by English? This repo is public, so that would be nice.

I’ve updated the descrption and a comment

mung9 avatar Sep 26 '24 02:09 mung9

~~I think it would be better to deprecate the old SlackReporter and separate it into SlackReporterV2. If there's a breaking change, We'll have to update the major version with the autopprof release, and I think it would be tiring to raise the major version because of subdependencies. Actually, github.com/slack-go/slack also created UploadFileV2Context and uploaded the minor version, not the major version. I'm curious to see what other people think.~~

https://github.com/daangn/autopprof/pull/18#discussion_r1776272231

MeteorSis avatar Sep 26 '24 02:09 MeteorSis

@mung9 I'm on about a one-year leave, so I'm not a member of daangn org at the moment 😅. But, of course, I can review the changes, and I'm happy to see that this pkg is being maintained well. I will leave the comments soon.

mingrammer avatar Sep 26 '24 02:09 mingrammer

@mung9 I'm on about a one-year leave, so I'm not a member of daangn org at the moment 😅. But, of course, I can review the changes, and I'm happy to see that this pkg is being maintained well. I will leave the comments soon.

@mingrammer You’ve been away for a long time! Thank you for taking the time to review despite that.

mung9 avatar Sep 26 '24 10:09 mung9

I have a quick question about CI configuration.

I noticed that our go.mod file specifies Go 1.19, but our CI pipeline also runs tests against older versions, including Go 1.16 to 1.17. Could you let me know the reasoning behind maintaining support for these earlier versions?

The reason I'm asking is that the tests are currently failing on the Go 1.16 run.

I'd appreciate your guidance on how to proceed. I see two main options:

We can fix the failing tests to ensure compatibility with Go 1.16. If this is the preferred path, I may need some help debugging the issue, as it seems specific to that version. Alternatively, if we no longer need to support this older version, we could update the CI configuration to remove the Go 1.16 test. Please let me know which direction you think is best.

Thanks!

mung9 avatar Jun 16 '25 08:06 mung9

This PR is quite old. Have there been any updates since the last review?

@winterjung Apologies for the delay. I've just pushed an update that incorporates the changes you suggested.

I've also added a separate comment regarding the failing backward compatibility tests for the older Go versions.

Please let me know what you think. Thanks!

mung9 avatar Jun 16 '25 08:06 mung9

Thanks for the fast feedback!

Okay, I'll drop support for Go 1.16 and note it in the changelog. The Go 1.16 tests have been removed.

I'll wait for any more comments until the end of the week and then merge

mung9 avatar Jun 17 '25 02:06 mung9