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

Speed up CI pipeline #1060

Merged
merged 33 commits into from
Jan 27, 2023
Merged

Speed up CI pipeline #1060

merged 33 commits into from
Jan 27, 2023

Conversation

KellyAH
Copy link
Contributor

@KellyAH KellyAH commented Jan 20, 2023

A continuation of #1053

Problem:

The CI pipeline currently runs as 1 job that executes everything sequentially:

  1. builds the app
  2. runs all tests

Fix:

Break up the pipeline so after the app is built, unit, and feature tests run in parallel.

  1. build app
  2. --> run unit tests
    --> run browser tests

TODO:

  • Cleanup yml
  • Break out separate sequential jobs - (passing run)
    • build - (can start rails app)
    • test-rspec - (enviro + run unit tests)
    • test-features - (browser enviro + run browser tests)
  • Make test jobs run in parallel if build job is successful
  • Fix Warning: The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
  • DRY up the jobs
    • extract repeated "services" sections into shared YAML block (see https://joshdevlin.com/blog/yaml-repeating-sections/) not supported by GitHub workflows
    • maybe look into reusing the steps that are part of "setup" somehow (either via YAML shared block or as a reusable workflow, or some other way)
    • maybe look into removing the extra health check options for postgres -- are those still needed?
    • look into updating the postgres version in CI -- make it match production and dev docker-compose version

runs-on: ubuntu-latest
needs: [build]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: needs: makes test job only execute if build job is successful.

- name: Start Rails server
run: |
export PATH=$PATH:~/bin
bin/run &
Copy link
Contributor Author

@KellyAH KellyAH Jan 20, 2023

Choose a reason for hiding this comment

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

note: build job currently has the same steps that test job does. but this is a WIP and I'll see if I can make setup a reusable step to DRY it up. Since we aren't using docker-compose to build the full stack i dunno it it's possible to build the app once and then share it between the different jobs. each job runs on its own VM so I think it's like each job is running on a fresh machine.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're already setting up caching, which should make downstream containers "speedrun" the bundle/yarn/brew processes!

So the most likely vector of decomposition would be to gather the duplicative bits into a job or pulling out a workflow but I'm not really fluent enough in Github Action's domain specific language to know which pathway is most likely to be shortest-to-successful.

# To wait for asset built
# TODO: Start server in production mode
curl --connect-timeout 5 --retry 5 --retry-delay 5 --retry-max-time 40 --retry-connrefused localhost:3000 1> /dev/null
bin/test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, all tests are running via bin/test script.

In order to decompose the test job into separate jobs test-features and test-rspec that will run in parallel, the pipe is gonna need a different command to execute tests.

I think these are the appropriate commands:

Copy link
Member

Choose a reason for hiding this comment

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

You could also split the bin/test script into bin/test-features and bin/test-rspec and then call those directly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe. But we use bin/test script for local test runs to conveniently run all the tests. If I split bin/test out into 2 new scripts, and we want to preserve the ability to easily run all tests that I'd have to... 🤔 make bin/test invoke bin/test-features and bin/test-rspec. Could it be an abstraction overkill?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's one of those tradeoffs where neither are advantageous over the other. Using bin/rspec or bin/rake for ruby and yarn run test for cucumber are totally fine.

@KellyAH KellyAH self-assigned this Jan 20, 2023
@zspencer
Copy link
Member

This is looking really good! It looks like the biggest time-savings will be removing homebrew from the CI pipeline; as that is adding 1.5m just to get homebrew installed, and another 4~5m to install the dependencies.

@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 21, 2023

This is looking really good! It looks like the biggest time-savings will be removing homebrew from the CI pipeline; as that is adding 1.5m just to get homebrew installed, and another 4~5m to install the dependencies.

I think we need brew for overmind and other dependencies.
Else overmind is not installed via bin/setup and rails doesn't start with bin/run. See failing run when brew was removed.

@zspencer
Copy link
Member

What if we... didn't use overmind or other brew-based installation bits in CI?

@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 25, 2023

I'm blocked on this. Waiting on a dev ops friend to advise me.

@zspencer
Copy link
Member

RSpec in < 1m! This is the happiest I've ever been!

@anaulin anaulin force-pushed the break-out-build-job branch from 03514fb to 38f882c Compare January 26, 2023 03:46
@anaulin
Copy link
Member

anaulin commented Jan 26, 2023

Getting very close. Things seem to be working in parallel, much faster than before (no brew installations), but we still have a couple of smallish thing to cleanup (see checkboxes in PR description).

image

@zspencer
Copy link
Member

4m!!! That's soooo fast!

@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 26, 2023

Thanks @anaulin and @zspencer for helping this work get unstuck and closer to the finish line 🏁 🐎 . I'll see if I can tackle the DRY tasks this week.

@KellyAH KellyAH changed the title add build job with test job dependent on it Speed up CI pipeline Jan 26, 2023
@anaulin
Copy link
Member

anaulin commented Jan 26, 2023

One thing I'm wondering about is that initial "install and cache dependencies" step: it does a bundle install and a yarn install, and that's it. The two test steps also have to then do bundle install and yarn install, and I don't know if the caches are shared between the different steps.

It might be better to just remove the separate "install" step, and just run the two different test sets in parallel.

Thoughts?

@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 26, 2023

One thing I'm wondering about is that initial "install and cache dependencies" step: it does a bundle install and a yarn install, and that's it. The two test steps also have to then do bundle install and yarn install, and I don't know if the caches are shared between the different steps.

It might be better to just remove the separate "install" step, and just run the two different test sets in parallel.

Thoughts?

🤔 What happens if we leave the "bundle install and a yarn install" steps in the setup job and remove them from the 2 parallel test jobs?

🗒️ According to GHA cache doc caching seems to be shared between jobs.

Use caching when you want to reuse files that don't change often between jobs or workflow runs, such as build dependencies from a package management system.

@anaulin anaulin force-pushed the break-out-build-job branch from 5d6cb8e to 6bfc432 Compare January 26, 2023 19:13
@anaulin
Copy link
Member

anaulin commented Jan 26, 2023

What happens if we leave the "bundle install and a yarn install" steps in the setup job and remove them from the 2 parallel test jobs?

I removed the yarn install steps from the test-_* jobs, and things still seem to work. I don't think I can remove the bundle install step without removing the "setup ruby" step, but I think that's ok, because it is takes about 5s (since things are cached):

image

@zspencer
Copy link
Member

So, is this ready to merge then?! It looks amazing! I want it!!!

@anaulin anaulin force-pushed the break-out-build-job branch from c205660 to 6e9d630 Compare January 27, 2023 21:55
@anaulin anaulin force-pushed the break-out-build-job branch from 6e9d630 to b0bfde4 Compare January 27, 2023 21:57
@anaulin
Copy link
Member

anaulin commented Jan 27, 2023

GitHub doesn't support YAML block reuse via anchors just yet:
image

So no "services" block reuse for us 🤷🏼‍♀️

@anaulin anaulin marked this pull request as ready for review January 27, 2023 21:59
@anaulin anaulin requested a review from a team January 27, 2023 22:00
@anaulin
Copy link
Member

anaulin commented Jan 27, 2023

@zspencer i think this is good enough to merge (if we want to do more tightening, we can always do that in follow-up PRs)

Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

YESSSSS

@anaulin anaulin merged commit 5e00038 into main Jan 27, 2023
@anaulin anaulin deleted the break-out-build-job branch January 27, 2023 22:11
@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 31, 2023

Thanks so much for taking this over the finish line @anaulin 🏁 🏎️ I ran outta steam at the end and this past week has been packed with family projects. Thanks again! 🙇‍♀️

@KellyAH
Copy link
Contributor Author

KellyAH commented Jan 31, 2023

wow the time-saving is amazing. before this change, pipeline took ~7m 47s, now it's 3m 37s. 🌟

@zspencer
Copy link
Member

This is the happiest I have ever been!

@zspencer zspencer added the 🛠️ infrastructure ci, build, deploy, networking, etc. label Apr 8, 2023
@zspencer zspencer added this to the 1.0 - Andromeda milestone May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ infrastructure ci, build, deploy, networking, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants