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

chore: Optimize CircleCI workflow #1919

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

sawadashota
Copy link
Contributor

@sawadashota sawadashota commented Jun 20, 2020

Proposed changes

Optimize CircleCI workflow to catch error and end faster.

CircleCI jobs work 4 jobs maximum at same time because of free OSS plan.
In hydra, there are 12 jobs to be kicked at created PR but actually just each random 4 jobs proceed.

I think there are 2 problems.
First, we cannot catch error faster if similar and long jobs such as test-e2e-* or test-legacy-* are kicked.
Second, error PR blocks other PR because concurrency limitation is shared. If on of test-e2e-* failed, others are likely to fail for same reason.

So I changed workflow like this

before
image

after
image

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

Signed-off-by: sawadashota <xiootas@gmail.com>
@sawadashota sawadashota force-pushed the optimize_ci_workflow branch from 2a26847 to bbf7c0b Compare June 20, 2020 04:15
@aeneasr
Copy link
Member

aeneasr commented Jun 20, 2020

Thank you! The problem really is CircleCI here which doesn't support canceling parallel jobs. Do you know if GitHub Actions does that? I think that the wait time is caused by the "executor" machine type which we currently use to check the legacy migrations. I was thinking of removing these tests once 1.5.2 is out because, unless we make changes, I don't think that there will be any issues with the new migration system.

@aeneasr
Copy link
Member

aeneasr commented Jun 20, 2020

I just checked and it appears that GitHub actions supports up to 20 concurrent workflows per repository, which would be much much more than we have on CircleCI right now. Since it's free for open source without hard usage limits (as opposed to CircleCI) it might be something we need to switch to. Would be a lot of work though :/

@sawadashota
Copy link
Contributor Author

Yeah, GitHub Actions seems better and it will need a lot of effort.
I couldn't find canceling parallel jobs spec or action but it seems it's possible if we use GitHub API.
https://developer.github.com/v3/actions/

- test-e2e-postgres
- test-e2e-mysql
- test-e2e-cockroach
- test-e2e-plugin
- test-legacy-migrations-postgres
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it still depend on indirectly. test-legacy-migrations-mysql and test-legacy-migrations-cockroach depend on test-legacy-migrations-postgres. So does test-e2e-memory.

@aeneasr
Copy link
Member

aeneasr commented Jun 22, 2020

Yeah I also saw that it's not possible to do manual job approval, which we currently use for sending out the newsletter :/

@aeneasr aeneasr merged commit 384f7ff into ory:master Jun 26, 2020
@aeneasr
Copy link
Member

aeneasr commented Jun 26, 2020

Thank you!

@sawadashota sawadashota deleted the optimize_ci_workflow branch June 26, 2020 09:35
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