-
Notifications
You must be signed in to change notification settings - Fork 619
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 reusable workflow for scheduled jobs to notify on slack. #3873
Conversation
7585014
to
4992682
Compare
.github/workflows/slack-notify.yaml
Outdated
status: ${{ job.status }} | ||
fields: repo,message,commit,author,action,eventName,ref,workflow,job,took,pullRequest | ||
env: | ||
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK }} |
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.
added this for a private ibc-e2e channel Carlos created for now.
small update, we recently updated the |
ee1c940
to
cdf9022
Compare
50e504e
to
00c9681
Compare
00c9681
to
2139430
Compare
.github/workflows/e2e-upgrade.yaml
Outdated
uses: ./.github/workflows/slack-notify.yaml | ||
secrets: inherit | ||
# Only send a slack notification if any of the tests failed, run only on cron jobs | ||
if: ${{ always() && contains(needs.*.result, 'failure') && github.event_name == 'schedule' }} |
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.
This works fine but there is a built in {{ failure() }}
example.
Should we use that instead?
Also, I don't think this lets us know which one failed (I don't think that's a huge problem since we will just click on the link and see for ourselves)
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.
This works fine but there is a built in {{ failure() }} example.
does this also apply to jobs? I remember this is an annoying discrepancy and using the contains with needs
was the only workaround to getting the behaviour "fail if any of these jobs failed".
I don't think this lets us know which one failed
Yup, tried to do that first go around. Requires additional inputs to re-usable workflow and explicit passing of secrets with inherit
, I didn't particularly like the way it was shaping out so went with the aggregate.
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.
Ah I missed that it was for a step and not a job! I'm not sure if it can be used the same way. Happy to leave it for now, either way it achieves the same result.
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.
LGTM left one suggestion, but always happy to update as needed 👍
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.
Did you ever manage to get this working? what channel would it post to?
Happy to approve if its g2g! :)
.github/workflows/e2e-upgrade.yaml
Outdated
@@ -131,3 +131,10 @@ jobs: | |||
relayer-type: rly | |||
relayer-image: ghcr.io/cosmos/relayer | |||
relayer-tag: latest | |||
|
|||
slack-notify: | |||
needs: [upgrade-v5-hermes, upgrade-v7-hermes, upgrade-v7_1-hermes, upgrade-v8-hermes, upgrade-v5-rly, upgrade-v7-rly, upgrade-v7_1-rly, upgrade-v8-rly] |
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 guess this is slightly stale now!
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.
lol. this did work but kinda lost most of its sense when we stopped nightly runs for relayer we don't use on each PR. Could still use it for upgrades if people would want it (after I freshen it up)
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.
Yeah maybe we need to re-evaluate what we would like to run on a cron schedule and see if people think it would be valuable to have a slack notification channel for it!
I can add it to the agenda for our next sync
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.
Running upgrade tests sounds fine. But there's not really much value in it if they're always using static image tags.
If we were running anything on a cron schedule it I think it should be at least picking up some latest code, build a fresh image and run a test harness against it.
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.
#5762 is a good reason to add slack notify workflow in. I've rm'ed the changes to upgrade
e2e file for time being, can add these at a later point.
@DimitrisJim Can we merge this one now? Or is there anything that we need to agree on or implement? |
think this should be fine @crodriguezvega! just adds the workflow! |
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.
Let's merge it!
Description
Pending discussion on which channel to post (configurable via the WebHook) and for which workflows (hermes cron job is one, might want one for
e2e-upgrade
which is also on a schedule).Current message drops a lot of information for starters, some of which might not really be needed (see a preview of it on the action repo here).
closes: #3842
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.