-
Notifications
You must be signed in to change notification settings - Fork 263
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
MAIN-2798 - Opsgenie slack #1673
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments, let me know what do you think.
logging.warning("No actions found in the Slack message.") | ||
return | ||
|
||
block_id = actions[0].get("block_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If in the future we will have mutlipe action what will happen? Why we aren't also checking the action name and making sure that this is indeed the action block and not only assume it is the first one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we can't find the right action using the text of something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name is not always guaranteed, the text here has no action as you see
The only thing guarenteed here is the block_id that was clicked in the callback
Also simplicity is key, if we overcomplicate it by searching for specific text and the text changes in runner that is worse than relying on the ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly i don't think it is worst, If you think it is high effort open ticket to support multiple action in that case and we will do it on next sprint.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small things.
this adds additional params to every action from a slack callback
slack_username
- the user that clicked on the slack callbackslack_message
- has metadata that can be used to change the slack message on button click