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

[CBT] Split test into multiple workflows #6555

Merged
merged 6 commits into from
May 15, 2023
Merged

[CBT] Split test into multiple workflows #6555

merged 6 commits into from
May 15, 2023

Conversation

ergo14
Copy link
Contributor

@ergo14 ergo14 commented May 1, 2023

  • With this, one failed container will flag the job as failed as soon as that container fails. This will show up on github and alert devs.
  • This will still not cancel the other containers. All workflows will still run. Defining each container job separately as a workflow does not change this (i.e. as opposed to what this PR does which is use matrix jobs)
  • It might be possible to get the cancelling behavior we want if we add a dummy job that depends on all tests. This has the extra cost of spinning up a new VM for nothing.

@ergo14 ergo14 requested a review from jeremyk-91 May 2, 2023 09:26
Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, one thing to think about in terms of the script

@@ -52,14 +52,14 @@ done

# Short circuit the build if it's docs only
if ./scripts/circle-ci/check-only-docs-changes.sh; then
if [ $CIRCLE_NODE_INDEX -eq 0 ]; then
if [ $1 -eq 0 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I agree that your change means that we reference the first arg of this script as opposed to the env variable, I think it's preferable to declare a variable in the script and then use it in named form elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants