Rewrite using buildkite-builder 💥✨ #73
Conversation
Also included the MYSQL_IMAGE and POSTGRES_IMAGE environment variables from before
Co-authored-by: Ngan Pham <[email protected]>
Copy of rails#72 for bkb branch Co-authored-by: Petrik <[email protected]>
Copy of rails#74 for bkb branch Co-authored-by: Hartley McGuire <[email protected]>
Originally I thought the bkb Lib extension adds lib path for you, so I would like to figure that out at some point /cc @ngan
This gives us another hook to refactor later, but also we can stub the artifacts in the tests so even if the version changes the tests shouldn't fail Co-authored-by: Ngan Pham <[email protected]>
|
@ngan Thanks for your review! I've applied your suggestions, and look forward to iterating on this together in the future 🙇 |
There was a problem hiding this comment.
This seems really cool! I left a few higher level comments and a few lower level comments about some things that stood out to me.
I wonder if its worth trying to simplify some of the version checks before making this change... we only run CI on 6.1+ at this point and there are a bunch of conditions checking for Rails 5.x or 6.0
lib/buildkite_config/docker_build.rb
Outdated
| "image-name" => build_context.ruby.image_name_for(build_context.build_id), | ||
| "cache-from" => [ | ||
| build_context.rebuild_id && "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for(build_context.rebuild_id), | ||
| build_context.pull_request && "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("pr-#{build_context.pull_request}"), | ||
| build_context.local_branch && build_context.local_branch !~ /:/ && "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("br-#{build_context.local_branch}"), | ||
| build_context.base_branch && "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("br-#{build_context.base_branch}"), | ||
| "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("br-main"), | ||
| ].grep(String).uniq, | ||
| push: [ | ||
| build_context.local_branch =~ /:/ ? | ||
| "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("pr-#{build_context.pull_request}") : | ||
| "base:" + build_context.image_base + ":" + build_context.ruby.image_name_for("br-#{build_context.local_branch}"), | ||
| ], |
There was a problem hiding this comment.
For the most part I like how a lot of the abstraction looks, but this section feels incredibly hard to grok. It seems like a lot of these methods end up passing a param from the build_context to another method on the build_context, could these type of methods just be lifted to the build_context completely?
| rubies.select { |r| !r.soft_fail? }.first | ||
| end | ||
|
|
||
| def setup_rubies(ruby_minors) |
There was a problem hiding this comment.
If I'm reading this correctly, this method takes ruby_minors as the only input and returns a list of RubyConfig? Feels like a class method on RubyConfig may be a better home, and it would allow us to split out some helper methods without polluting BuildContext as well (there's a lot going on in here)
|
I'm trying this approach so we go with it. I'll work on top of this branch |
This is more like the default_ruby to use in pipelines that don't need to run against multiple rubies.
Let's upgrade outside of the refactoring.
| plugin :docker_compose, { | ||
| "env" => %w[PRE_STEPS RACK], | ||
| "run" => service, | ||
| "pull" => service, |
There was a problem hiding this comment.
Do we actually need that pull? Looking at builds, every single job spend 30-40 seconds pulling images that are already there locally:
I get that if these images were to change in the registry we'd risk having some workers run outdated images and some up to date ones, but that's probably an acceptable tradeoff to speed CI by this much, no?
There was a problem hiding this comment.
NB: I'm quite familiar with docker, but pretty much not at all with docker-compose so perhaps I'm misunderstanding the purpose of that pull.
There was a problem hiding this comment.
@byroot My naive understanding is that is there for downstream jobs to pull the base image from the build job, and pull is there to make sure we get the right one. But to be honest, I was only hoping to achieve as close to 1:1 with the existing pipeline before making any changes, so there is definitely room for improvement.
If you want to open a PR to here to confirm it, you can run a test build of the pipeline like here by click the "Review Build Script".
There is a build triggered on PRs to this repo, but it requires approval from a team member to run, wasn't sure if you knew about this yet. 🙇
Here is the initial refactor to use buildkite-builder.
Things to note
BuildContextextension here:DockerBuildextension here:step_foris now theRakeCommandextension here:meta-ci
You'll also notice there are a lint and test jobs in the buildkite-config pipeline:
https://buildkite.com/rails/config-sandbox/builds/70
These join the existing annotate job, which was refactored to use the same unprivileged docker container strategy as pipeline-generate.
Here is a rails/rails (main) build using the new pipeline format:
https://buildkite.com/rails/rails-sandbox/builds/56
I haven't yet tested this with branches and PRs since, probably the biggest risk there is how
docker builduses the branches and pr info to generate the build cache.