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

#73 added thread-ts as an output #74

Merged
merged 2 commits into from
May 4, 2022
Merged

#73 added thread-ts as an output #74

merged 2 commits into from
May 4, 2022

Conversation

stevengill
Copy link
Member

@stevengill stevengill commented Mar 9, 2022

Summary

Fixes #73 by adding thread_ts as an output when using a botToken. This is useful for threading.

Requirements (place an x in each [ ])

@stevengill stevengill added this to the 1.19 milestone Mar 9, 2022
@stevengill stevengill requested a review from seratch March 9, 2022 01:20
@stevengill stevengill self-assigned this Mar 9, 2022
action.yml Outdated
@@ -16,6 +16,8 @@ inputs:
outputs:
time: # id of output
description: 'The time'
message-ts: # timestamp on the message that was posted when using bot token
Copy link
Member

Choose a reason for hiding this comment

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

I have two things that I would like to discuss here:

1: always passing thread_ts?
What do you think about naming this thread-ts instead and making the slack-send to always return thread-ts? This means that if you run the step twice, you can easily post a message in the same thread. I cannot think of the use cases to prefer knowing message.ts instead of message.thread_ts.

The possible use case would be:

steps:
  - uses: slackapi/[email protected]
    id: slack1
    with:
      channel-id: '#general'
      slack-message: "This is the first message"
  - uses: slackapi/[email protected]
    id: slack2
    with:
      channel-id: '#general'
      payload: |
        {
          "text": "The first reply",
          // Actually this is parent message's message.ts but the following steps use it as thread_ts
          "thread_ts": "${{ steps.slack1.outputs['thread-ts'] }}" 
        }
  - uses: slackapi/[email protected]
    id: slack3
    with:
      channel-id: '#general'
      payload: |
        {
          "text": "The second reply",
          // This should be the same ts with slack1's message ts
          "thread_ts": "${{ steps.slack2.outputs['thread-ts'] }}"
        }

2: thread-ts vs thread_ts?

I know using dash as delimiter is consistent with the existing input value keys. That being said, using underscopre can be a bit similar for users. In the YAML expression, the difference would be steps.slack1.outputs['thread-ts'] vs steps.slack1.outputs.thread_ts.

@@ -114,6 +116,10 @@ module.exports = async function slackSend(core) {

const time = (new Date()).toTimeString();
core.setOutput('time', time);

if (webResponse && webResponse.ts !== undefined) {
core.setOutput('message-ts', webResponse.ts);
Copy link
Member

Choose a reason for hiding this comment

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

If you return thread-ts instead, we can check if there is thread_ts first and then fall back to message.ts. Also, it may be fine to return both message-ts and thread-ts.

@stevengill stevengill changed the title #73 added message-ts as an output #73 added thread-ts as an output Mar 18, 2022
@stevengill
Copy link
Member Author

@seratch updated this PR. Let me know

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

The changes look good to me. How about adding a following step to post a reply in thread here:

integration_test_botToken:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- run: npm ci && npm run build
- name: Post message to Slack via botToken
id: slackToken
uses: ./
with:
channel-id: ${{ secrets.SLACK_CHANNEL_ID }}
slack-message: 'CI Post from slack-send GitHub Action! Succeeded!!'
# payload: "{\"key\":\"value\",\"foo\":\"bar\"}"
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}
# Use the output from the `slackToken` step
- name: Check Action output is not empty
run: test -n "${{ steps.slackToken.outputs.time }}"

@lenamikaze
Copy link

Will this be merged in anytime soon?

@SnowCait
Copy link

SnowCait commented May 3, 2022

Hi @seratch, @stevengill

What blocks this PR from merge?

@stevengill stevengill added the enhancement New feature or request label May 4, 2022
@stevengill stevengill merged commit 3b53905 into main May 4, 2022
@stevengill stevengill deleted the issue-73 branch May 4, 2022 06:08
@stevengill
Copy link
Member Author

Sorry for they delay! I've merged this in and released version V1.19.0

@SnowCait
Copy link

SnowCait commented May 4, 2022

@stevengill Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support response output
4 participants