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

Added support to TS and Footer on error #89

Merged
merged 1 commit into from Nov 24, 2023

Conversation

extmkv
Copy link
Contributor

@extmkv extmkv commented Sep 13, 2023

Checklist

  • I've read and followed the Contribution Guidelines
  • step.yml and README.md is updated with the changes (if needed)

Version

Requires a MINOR version update

Context

Adds support to update messages using slack step.
Also adds support to the footer message and icon on error.

Changes

Added 3 new properties:

ts: When ts is provided and when using api_token it will update the original message with new content.
To understand how the slack update method works, please check here.

 footer_on_error: Works in the same way as the others on error.
footer_icon_on_error: Works in the same way as the others on error.

Investigation details

N/A

Decisions

@extmkv extmkv changed the title Added support to TS Added support to TS and Footer on error Sep 13, 2023
@extmkv
Copy link
Contributor Author

extmkv commented Oct 5, 2023

@hisaac can you have a look?

@hisaac
Copy link
Contributor

hisaac commented Oct 5, 2023

Hi @extmkv, I don't work on this anymore so I won't be able to help. Perhaps @DamienBitrise could help, or recommend someone who could review?

@RamithaW
Copy link

RamithaW commented Oct 9, 2023

@DamienBitrise is this something you can help review? We would love to have have this feature on the bitrise-steplib.

Copy link

@godrei godrei left a comment

Choose a reason for hiding this comment

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

Hi @extmkv, sorry for the late response.
I reviewed the code changes and have a few comments, please check it out.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
step.yml Show resolved Hide resolved
step.yml Outdated Show resolved Hide resolved
@extmkv
Copy link
Contributor Author

extmkv commented Nov 17, 2023

@godrei I've fixed your comment.

@godrei
Copy link

godrei commented Nov 24, 2023

Hi @extmkv , thanks for the changes.
I did some manual testing, to try out the new feature and I faced an interesting difference between the chat.postMessage and chat.update endpoints.

I used this workflow to post a message to a channel and then update the original message:

  slack:
    steps:
    - git::https://github.com/extmkv/steps-slack-message.git@master:
        title: Send message to be updated
        is_skippable: false
        inputs:
        - api_token: "$SLACK_API_TOKEN"
        - channel: "$SLACK_CHANNEL"
        - from_username: step-dev-test
        - text: Testing slack message update
        - output_thread_ts: THREAD_TS
        - is_debug_mode: 'yes'
    - git::https://github.com/extmkv/steps-slack-message.git@master:
        title: Update message
        is_skippable: false
        inputs:
        - api_token: "$SLACK_API_TOKEN"
        - channel: $SLACK_CHANNEL
        - from_username: step-dev-test
        - text: Slack message updated
        - ts: "$THREAD_TS"

The messages are connected through the THREAD_TS output of the first step. I expected the original message's text to change from Testing slack message update to Slack message updated.

What I experienced is that the first message was posted, but it wasn't updated, even if both steps were successful.
After adding some debug logging around the HTTP calls, it turned out that the client returned status OK for the update message with the following body:

{"ok":false,"error":"channel_not_found"}

And here was the source of the issue: I used a channel's name as the channel input for the step and it was working fine for posting a message, but the update endpoint requires a channel ID.

After updating the channel step input, I managed to verify the new functionality, so we can merge and release the changes.
I will try to allocate some time to improve how the message posts and updates are verified.

@godrei godrei merged commit 73c8736 into bitrise-steplib:master Nov 24, 2023
1 check passed
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.

None yet

4 participants