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

Consolidate Discord notification steps #4088

Merged
merged 1 commit into from
May 19, 2024
Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 18, 2024

Motivation

After #4082, I wanted to look for ways to set the first parameter of the Discord notification script (success or failure) without environment variables, extra pseudo-steps ("Set Success"), or side effects. Gradually I realized that writing a git clone command in every place we want to notify was a bit duplicative and could use better encapsulation.

Changes

  • Now our own notify.yml action wrapper provides the nice API that DiscordHooks/github-actions-discord-webhook should have.
    • Inputs name and success are accepted, and the DISCORD_WEBHOOK secret is required.
    • The somewhat awkward git clone logic required to use it is now centralized in one place. If the optimal notification method changes in the future, we will be able to update it in just one spot.
  • Now what the action documentation calls "ternary operator like behaviour" is used to choose between success and failure based on the value of the boolean success input, with no environment variable twiddling involved.
  • Now all spots that previously sent their own notifications are updated to go through the new notify.yml, and the contains expression usees the ! not operator instead of == false.
  • The notify jobs in test.yml and smoke.yml now depend on their respective build-* steps to ensure that failure can be detected by checking contains(needs.*.result, 'failure') (if you only needs the intermediate step, its status will be skipped when a previous step fails).

This should be a bit more maintainable, and it gave me some more practice in action coding.

@HebaruSan HebaruSan added Build Issues affecting the build system Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) labels May 18, 2024
@HebaruSan HebaruSan force-pushed the refactor/notify-workflow branch from 80644cd to da539c4 Compare May 18, 2024 23:11
@HebaruSan HebaruSan added the In progress We're still working on this label May 18, 2024
@HebaruSan HebaruSan force-pushed the refactor/notify-workflow branch 7 times, most recently from d1eec18 to 06da3ee Compare May 19, 2024 00:26
@HebaruSan HebaruSan force-pushed the refactor/notify-workflow branch 2 times, most recently from f6cca93 to 897fde5 Compare May 19, 2024 00:42
@HebaruSan HebaruSan merged commit e00cf14 into master May 19, 2024
6 checks passed
@HebaruSan HebaruSan deleted the refactor/notify-workflow branch May 19, 2024 00:49
@techman83
Copy link
Member

Love this! It was on my hit list to look at 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Issues affecting the build system In progress We're still working on this Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants