-
Notifications
You must be signed in to change notification settings - Fork 889
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
Update CI Triggers #2763
Update CI Triggers #2763
Conversation
.github/workflows/test-all.yml
Outdated
@@ -1,11 +1,7 @@ | |||
name: Run All Tests |
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.
Since the other workflow is renamed to "Test Modified Packages", how about also renaming this one to "Test All Packages"?
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.
+1
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.
Done.
.github/workflows/test-all.yml
Outdated
- master | ||
types: | ||
- closed | ||
push: | ||
schedule: | ||
- cron: '0 6,18 * * *' |
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.
Do we still need the cron job?
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 it hasn't been very helpful. Removing.
.github/workflows/test-changed.yml
Outdated
pull_request: | ||
branches: | ||
- '*' | ||
- '!master' |
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.
What does excluding master for PR mean? Don't we always create PR against master?
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.
Yes, I think I was confused about what "branches" means for PR. Fixing.
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'm not a code owner, but LGTM!
Previously:
Now:
Additionally, we will change the repo settings to ensure "Test Modified Packages" is a required check and "Run All Tests" is optional, so if it runs too many tests and fails a flaky test, it does not block merge.