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

run GHA on push #1373

Merged
merged 2 commits into from
Feb 17, 2022
Merged

run GHA on push #1373

merged 2 commits into from
Feb 17, 2022

Conversation

spring1843
Copy link
Contributor

@spring1843 spring1843 commented Feb 17, 2022

1. Issue, if available:

2. Description of changes:

This will cause https://github.com/aws/karpenter/blob/main/.github/workflows/ci.yaml to run not only on pull_requests but also on push.

A few reasons this is a good idea

  • It will update the coverage data of the main branch in coveralls.io
  • GH will add a check icon to the commit, we currently have that since this but with this change the check will be more meaningful as it would mean we have done all our tests on this specific commit

It will also have some other less important but possible effects

  • An administrator can sometimes ignore the checks in a PR (a Github setting can disable this), the tests will still run and have a sign indicating that the commit is breaking the tests in such case
  • Some race conditions are not detected every time the code runs in Go. The race detector would (probably) need to observe the occurrence of a race to be able to detect it, if the program logic does not always lead to that occurrence then the race condition will remain undetected, either due to internal mechanics of race detection or the way the application code behaves the race condition detection is improved when you run the tests multiple times rather than once. So running the tests one more time after merge can help us detect such cases
  • A commit can sometimes be pushed into the main branch without a PR with a direct push to main (a Github setting can disable this), our checks would still run against that commit
  • There can be a drift in dependencies from the time the PR checks ran to the time we merged the PR, for example one of them maybe removed, compromised etc... With this change would know about that after we merge the PR.
  • There can be a change in other parts of the code base from the time PR checks ran to the time we merged. I'm not sure that Github invalidates the PR checks if another file in the repo is modified in main branch or not; This could mean some tests passed at the time the PR checks ran but not anymore after we merge. With this check we will not be taking that risk.

So it does somewhat add to our stability by doing the checks one more time!

3. How was this change tested?

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Feb 17, 2022

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 3f2b592

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/620ecd7a0b7fc7000856453a

@spring1843 spring1843 merged commit b7686fa into main Feb 17, 2022
@spring1843 spring1843 deleted the rm/run-on-push branch February 17, 2022 23:11
@spring1843 spring1843 mentioned this pull request Feb 17, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants