-
Notifications
You must be signed in to change notification settings - Fork 6
Fix health check #39
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
Fix health check #39
Conversation
@@ -33,6 +33,7 @@ jobs: | |||
- name: Notify slack | |||
uses: kpritam/slack-job-status-action@v1 | |||
with: | |||
if: ${{ failure() }} |
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.
@kleinjm I'm pretty sure that this will work fine with this and notify us of failure.
I would rather we trust the system and be notified only of failures in order to diminish noise.
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.
absolutely. And FWIW, that artifact upload GH action uses failure()
and works as expected (doesn't try to upload on success case) so this should work
job-status: ${{ job.status }} | ||
slack-bot-token: ${{ secrets.SLACK_BOT_TOKEN }} | ||
channel: github-actions |
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.
are you going to delete this channel? I'm in favor of that
.github/workflows/health_check.yml
Outdated
node: ['10', '12', '14'] | ||
node: ['12'] |
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.
not sure you want to do this if we care to support all versions. You can set max-parallel: 1
to run these in series instead of parallel and then we'll only be alerted on the first node version test suite that fails. See https://github.com/patch-technology/patch-ruby/blob/4c8f0f5fd64a46f4e4815764be3ffb677be425db/.github/workflows/test.yml#L16
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.
Good idea. I was concerned about getting four useless success notifications, but now that I've switched to failure only, this should work.
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.
not blocking but I feel we should keep all node versions
What
Why
SDK Release Checklist