Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

slack: add file upload functionality #9472

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

slack: add file upload functionality #9472

wants to merge 2 commits into from

Conversation

Powma
Copy link

@Powma Powma commented Dec 30, 2024 •
edited by felixfontein
Loading

SUMMARY

Add file upload functionality to upload the files with the new API SLACK.

Continuation of #9431.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • Slack
ADDITIONAL INFORMATION

This enhancement allows you to upload files with the new version of the Slack API
https://api.slack.com/methods/files.completeUploadExternal
https://api.slack.com/methods/files.getUploadURLExternal

Example usage:

    - name: Upload a file to Slack
      community.general.slack:
        token: thetoken/generatedby/slack
        channel: 'kuma'
        upload_file:
          path: "/tmp/test.py"
          title: "main.py"
         snippet_type: "python"

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Dec 30, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot
Copy link
Collaborator

cc @ramondelafuente
click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) labels Dec 30, 2024
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 30, 2024
russoz
russoz reviewed Dec 30, 2024
View reviewed changes
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Powma thanks for your contribution!

Other than fixing the sanity checks, and adding the version in the docs, it LGTM

plugins/modules/slack.py
thread_ts:
type: str
description:
- Optional timestamp of parent message to thread this message, see U(https://api.slack.com/docs/message-threading).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it makes the cut today, else we'll have to update the version number

Suggested change
- Optional timestamp of parent message to thread this message, see U(https://api.slack.com/docs/message-threading).
- Optional timestamp of parent message to thread this message, see U(https://api.slack.com/docs/message-threading).
version_added: 10.2.0

felixfontein
felixfontein reviewed Dec 30, 2024
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

plugins/modules/slack.py
Comment on lines +151 to +152
- See Slack's file upload API for details at U(https://api.slack.com/methods/files.getUploadURLExternal).
- See Slack's file upload API for details at U(https://api.slack.com/methods/files.completeUploadExternal).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are two paragraphs with the same text, but different URLs. Either write something like

Suggested change
- See Slack's file upload API for details at U(https://api.slack.com/methods/files.getUploadURLExternal).
- See Slack's file upload API for details at U(https://api.slack.com/methods/files.completeUploadExternal).
- See Slack's file upload API for details at U(https://api.slack.com/methods/files.getUploadURLExternal) or
U(https://api.slack.com/methods/files.completeUploadExternal).

or adjust the texts.

plugins/modules/slack.py
upload_url_data = json.load(response)
except ValueError:
module.fail_json(
msg="The Slack API response is not valid JSON: %s" % response.read()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point the response has likely already been read. You should read it first and then use the result both to decode JSON and to report the error here, otherwise this reporting here won't work.

plugins/modules/slack.py
type: str
description:
- Optional title for the uploaded file.
thread_ts:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this option relate to the global thread_id?

plugins/modules/slack.py

if upload_file:
try:
upload_response = upload_file_to_slack(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this seems to be very separate from the rest of the module. Maybe this should be a new module that only does file uploading, instead of squeezing this into the current module which is about posting messages?

changelogs/fragments/slack.yml
@@ -0,0 +1,2 @@
minor_changes:
- Slack upload file - feature added support for uploading files to Slack (https://github.com/ansible-collections/community.general/pull/9472).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Slack upload file - feature added support for uploading files to Slack (https://github.com/ansible-collections/community.general/pull/9472).
- slack - add support for uploading files to Slack (https://github.com/ansible-collections/community.general/pull/9472).

@russoz russoz changed the title feat(slack) : Add file upload functionality slack: add file upload functionality Jan 2, 2025
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

@russoz russoz russoz left review comments

Assignees
No one assigned
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Powma @ansibullbot @russoz @felixfontein