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.

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

add support for unfurl_links + better logging #3

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Conversation

dbertram
Copy link
Member

@dbertram dbertram commented Nov 7, 2024

  • add support for optional unfurl_links argument (defaults to true, but can be used to disable link unfurling when desired)
  • use --fail-with-body so HTTP status of >= 400 triggers an error exit code
  • use long form curl args in general for better readability/comprehensibility
  • break up curl args over multiple lines to improve readability
  • log raw response before processing it
  • fix how this action sets its ts output so that it actually works!
  • adds a GH workflow to this repo to enable testing changes in PRs before they're merged and without having to setup private, throw-away repos that use your branch of this GH action for testing

- use --fail-with-body so HTTP status of >= 400 triggers an error exit code
- use long form curl args in general for better readability/comprehensibility
- break up curl args over multiple lines to improve readability
- log raw response before processing it
- tweak multi-line GITHUB_OUTPUT capture to more closely match doc examples
@dbertram dbertram requested a review from bradymholt November 7, 2024 20:56
action.yml Outdated
Comment on lines 27 to 40
echo "ts=<<EOF"
curl --silent --show-error -X POST -H "Authorization: Bearer ${{ env.SLACK_BOT_TOKEN || inputs.token }}" -H "Content-Type: application/json; charset=utf-8" --url https://slack.com/api/chat.postMessage \
-d "{\"channel\": \"${{ inputs.channel }}\", \"thread_ts\":\"${{ inputs.thread_ts }}\", \"text\":\"${{ inputs.text }}\"}" \
| jq --raw-output '.ts'
echo "ts<<EOF"
echo "$response" | jq --raw-output '.ts'
Copy link
Member Author

Choose a reason for hiding this comment

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

Something about how I was trying to output the ts output variable wasn't playing nicely with what GH was expecting:

image

I was looking more closely at their examples for how to do multi-line output and noticed I had added the equal sign in ts=<<EOF that wasn't in their example: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#multiline-strings

...but I'm not sure how test test this assumption before merging this PR. 😟

Copy link
Member

Choose a reason for hiding this comment

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

You could create a workflow in a throw-away repo that references this branch like this:

- name: "Post Slack message"
  id: slack-post
  uses: ynab/slack-post-message-action@db/better-logging
  with:
    channel: "#my-channel"
    text: "Test 123"
- run: echo ${{ steps.slack-post.outputs.ts }}

Copy link
Member Author

Choose a reason for hiding this comment

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

The bigger issue here was that I needed to define how the ts output gets its value. The Invalid format aspect did need to be fixed, but didn't actually correct the problem I was debugging: the ts output not being available in later steps for actions using this shared action.

--header "Content-Type: application/json; charset=utf-8" \
--url https://slack.com/api/chat.postMessage \
--data "{\"channel\": \"${{ inputs.channel }}\", \"thread_ts\": \"${{ inputs.thread_ts }}\", \"unfurl_links\": ${{ inputs.unfurl_links }}, \"text\": \"${{ inputs.text }}\"}" \
)
Copy link
Member Author

Choose a reason for hiding this comment

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

My goals with these changes are:

  • make this step fail if we get a response with HTTP status >= 400 (i.e., didn't work)
  • capture the response and log it (to make debugging easier if anything does go wrong)
  • set the ts output for this action by processing the curl response

But I'm a total bash/curl newb, so please feel free to make suggestions! 🙏

Copy link
Member

@bradymholt bradymholt left a comment

Choose a reason for hiding this comment

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

Looks great. I posted a few comments but nothing to hold this up.

unfurl_links:
description: "Whether links in the message should be unfurled"
type: boolean
default: true
Copy link
Member

Choose a reason for hiding this comment

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

Does this default to true in Slack API if not provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

Their docs don't actually say what it defaults to, but my anecdotal testing seems to indicate yes

action.yml Outdated Show resolved Hide resolved
action.yml Outdated
Comment on lines 27 to 40
echo "ts=<<EOF"
curl --silent --show-error -X POST -H "Authorization: Bearer ${{ env.SLACK_BOT_TOKEN || inputs.token }}" -H "Content-Type: application/json; charset=utf-8" --url https://slack.com/api/chat.postMessage \
-d "{\"channel\": \"${{ inputs.channel }}\", \"thread_ts\":\"${{ inputs.thread_ts }}\", \"text\":\"${{ inputs.text }}\"}" \
| jq --raw-output '.ts'
echo "ts<<EOF"
echo "$response" | jq --raw-output '.ts'
Copy link
Member

Choose a reason for hiding this comment

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

You could create a workflow in a throw-away repo that references this branch like this:

- name: "Post Slack message"
  id: slack-post
  uses: ynab/slack-post-message-action@db/better-logging
  with:
    channel: "#my-channel"
    text: "Test 123"
- run: echo ${{ steps.slack-post.outputs.ts }}

@dbertram dbertram force-pushed the db/better-logging branch 7 times, most recently from c252536 to a432e4c Compare November 14, 2024 20:01
@dbertram dbertram force-pushed the db/better-logging branch 4 times, most recently from 7458cdb to 7d6219c Compare November 25, 2024 20:08
- test sending a Slack message with all optional inputs using their default values
- test sending a Slack message with all optional inputs provided (reply message + link unfurling disabled)
action.yml Outdated Show resolved Hide resolved
action.yml Show resolved Hide resolved
action.yml Show resolved Hide resolved
action.yml Outdated
Comment on lines 27 to 40
echo "ts=<<EOF"
curl --silent --show-error -X POST -H "Authorization: Bearer ${{ env.SLACK_BOT_TOKEN || inputs.token }}" -H "Content-Type: application/json; charset=utf-8" --url https://slack.com/api/chat.postMessage \
-d "{\"channel\": \"${{ inputs.channel }}\", \"thread_ts\":\"${{ inputs.thread_ts }}\", \"text\":\"${{ inputs.text }}\"}" \
| jq --raw-output '.ts'
echo "ts<<EOF"
echo "$response" | jq --raw-output '.ts'
Copy link
Member Author

Choose a reason for hiding this comment

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

The bigger issue here was that I needed to define how the ts output gets its value. The Invalid format aspect did need to be fixed, but didn't actually correct the problem I was debugging: the ts output not being available in later steps for actions using this shared action.

@dbertram dbertram merged commit 1627f99 into main Nov 26, 2024
1 check passed
@dbertram dbertram deleted the db/better-logging branch November 26, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants