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

Add reusable workflow for scheduled jobs to notify on slack. #3873

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

DimitrisJim
Copy link
Contributor

@DimitrisJim DimitrisJim commented Jun 16, 2023

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

type: commit message

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.

  • Targeted PR against correct branch (see CONTRIBUTING.md).
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@DimitrisJim DimitrisJim added the github_actions Pull requests that update Github_actions code label Jun 19, 2023
status: ${{ job.status }}
fields: repo,message,commit,author,action,eventName,ref,workflow,job,took,pullRequest
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK }}
Copy link
Contributor Author

@DimitrisJim DimitrisJim Jul 3, 2023

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.

@chatton
Copy link
Contributor

chatton commented Oct 4, 2023

small update, we recently updated the upgrade workflows to run when the files/workflows related to upgrades have changed. It's probably still no harm to run them on a schedule if we want think we want to.

@DimitrisJim DimitrisJim force-pushed the slack-notify-workflow branch 9 times, most recently from ee1c940 to cdf9022 Compare November 2, 2023 02:18
@DimitrisJim DimitrisJim force-pushed the slack-notify-workflow branch 3 times, most recently from 50e504e to 00c9681 Compare November 2, 2023 11:50
@DimitrisJim DimitrisJim marked this pull request as ready for review November 2, 2023 12:22
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' }}
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

@chatton chatton left a 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 👍

@crodriguezvega crodriguezvega added priority PRs that need prompt reviews and removed priority PRs that need prompt reviews labels Jan 15, 2024
Copy link
Member

@damiannolan damiannolan left a 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! :)

@@ -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]
Copy link
Member

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!

Copy link
Contributor Author

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)

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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.

@crodriguezvega
Copy link
Contributor

@DimitrisJim Can we merge this one now? Or is there anything that we need to agree on or implement?

@DimitrisJim
Copy link
Contributor Author

think this should be fine @crodriguezvega! just adds the workflow!

Copy link
Contributor

@crodriguezvega crodriguezvega left a 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!

@DimitrisJim DimitrisJim merged commit d266170 into main Feb 20, 2024
65 checks passed
@DimitrisJim DimitrisJim deleted the slack-notify-workflow branch February 20, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add slack webhook notifications for e2e nightly cron jobs
4 participants