CircleCI-Public / slack-orb

Create custom Slack notifications for CircleCI job statuses
https://circleci.com/developer/orbs/orb/circleci/slack
MIT License
213 stars 205 forks source link

can we add support to slack file upload? #52

Closed sibelius closed 4 years ago

sibelius commented 5 years ago

Orb version

2.1

What happened

There is no way to upload a file to slack

Expected behavior

slack/upload command should be available

How I do today

Install slack cli

brew install jq && curl -O https://raw.githubusercontent.com/entria/slack-cli/master/src/slack && chmod +x slack

upload file

slack file upload \
              --file packages/app/android/app/build/outputs/apk/alpha/app-alpha.apk \
              --filetype apk \
              --title "production - ${CIRCLE_TAG}" \
              --comment "New production apk available! - ${CIRCLE_TAG}" \
              --channels '#channel'
iynere commented 5 years ago

this is a great idea ! @sibelius feel free to submit a pull request—looks like you've got the syntax worked out already, basically 😄

sibelius commented 5 years ago

you are using pure curl

we are using this helper https://github.com/rockymadden/slack-cli

makes sense to have this helper in this package?

or just copy and paste curl syntax for upload?

iynere commented 5 years ago

i'd prefer to have fewer external dependencies, so it's most widely accessible @sibelius that project looks cool / helpful tho !

sibelius commented 5 years ago

is there any way to test if locally?

or is there something like jest for orbs?

iynere commented 5 years ago

@sibelius if you open a PR i'll merge it to our staging branch, which will automatically run tests against our own Slack

dotslash21 commented 4 years ago

Hi folks! I'd like to give this a try if no one is currently working on this issue. I'm a beginner and discovered this repo through hacktoberfest please guide me on how can I contribute here.

dotslash21 commented 4 years ago

I've created a draft pull request, can anyone check it out and help me confirm if I'm on the correct path?

KyleTryon commented 4 years ago

Im open to hearing from other people but I think it might make the most sense to add "upload" as a parameter to the "notify" command. And supply that parameter with the file path in your config. @dotslash21 if you want to try that it should actually be even easier. You can modify the existing notify command CURL request to include the upload parameter (will need to check slack's API to verify exactly what that looks like).

We can add a new command as well but I don't think it is needed.

dotslash21 commented 4 years ago

@KyleTryon I did some digging and I found out that message-attachment API of slack doesn't support file attachment. To upload files there is 'files.upload' method to upload a file to slack channels. If I add "upload" as a parameter to the "notify" command, then I'd need to curl to two different links with each having different parameters. This would make the file a bit complex. What do you think?

KyleTryon commented 4 years ago

I think we may want to consider rebuilding the slack orb utilizing a CLI such as @sibelius has mentioned. We will need to assess how portable it is, determine what it may not be compatible with. We will revisit this as soon as we are able to.

gmemstr commented 4 years ago

I'm going to opt to leave this as a "won't fix", for a few reasons.

  1. The Slack CLI, while very helpful, is not a first party project and would require us to verify every version. Where possible, we prefer to use first party or trusted utilities. While I'm sure the project in question wouldn't do anything malicious, it's additional responsibility on us.
  2. We want to keep this orb relatively portable with little overhead, and should ideally run on all official runtimes offered by CircleCI. Introducing the CLI may add additional complexities in this regard (esp. Windows).

My recommendation would be leveraging the rewrite (see #160) and saving the required files as artifacts, providing a link to the job page within a custom message.