-
Notifications
You must be signed in to change notification settings - Fork 1
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
DR-3406: Update slack notify action: Correct branch; Move status check to action call #1588
Conversation
Passing run #3221 ↗︎
Details:
Review all test suite changes for PR #1588 ↗︎ |
e47ba6d
to
de2a420
Compare
@@ -102,6 +102,8 @@ jobs: | |||
needs: [ update_image, helm_tag_bump, cherry_pick_image_to_production_gcr ] | |||
uses: ./.github/workflows/notify-slack.yaml | |||
secrets: inherit | |||
if: ${{ !cancelled() }} | |||
with: | |||
workflow_name: Dev Image Update | |||
notify_on_success: true |
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.
Does this parameter still make sense? In the false case wouldn't the workflow no longer be called?
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 are three states that we can check for here - success, failure or cancelled. (Cancelled being when the job is canceled manually by a user). So, the !cancelled() check means that it will run either on success on failure.
It's a little clunky, but it is the suggested option in the github actions docs.
If you want to run a job or step regardless of its success or failure, use the recommended alternative: if: ${{ !cancelled() }}
But, given that it's confusing, I think I'll just switch to always() and then cancelled jobs will just get the failure notification.
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.
Looks good to me
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.
I had the same question as Phil, but besides that this looks good!
Correct branch for jade-data-ui
de2a420
to
a52e5d8
Compare
689c53d
to
e140272
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
The changes
-> I don't love the way we're now passing the status: "needs.update_image.result == 'success'" appears to be the right move from some online research but definitely is verbose 😞. We don't have access to the "success()" and "failure()" functions when setting the arguments (at least as far as I can tell).
Testing
With change - successfully sending failure notification on job failure:
and success notification on job success: