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

Stable gemset for integration testing #1470

Merged
merged 11 commits into from
Apr 21, 2021
Merged

Stable gemset for integration testing #1470

merged 11 commits into from
Apr 21, 2021

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Apr 19, 2021

This PR creates two distinct CircleCI pipelines for ddtrace:

  • build-and-test: executes all tests using a stable gemset. Runs on every commit. Generates coverage report and prerelease artifacts.
  • edge: executes all tests using the latest available gems. Runs once per day.

This should allow us to have a much more stable CI pipeline for daily development, while also being aware of any issues created by new external gem releases.

To accomplish this, all Gemfiles and lock files generated by Appraisals are now checked in the repository. Because Appraisal doesn't allow changing the destination directory of the generated gemfiles (I tried to modify the gem to support it, but ended up with mixed results), and it generates overlapping names (./gemfiles/contrib.gemfile for all versions of Ruby, for example), I've introduced some code in Appraisal and Rakefile to ensure that each gemset has a unique name.
This should have to effect on daily development workflow.

One unintended upside is that CI build steps are much faster: around 1 minute or less for CRuby variants.

@marcotc marcotc self-assigned this Apr 19, 2021
ivoanjo
ivoanjo previously approved these changes Apr 20, 2021
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Looks good to me! Seems like a reasonable balance between getting advance warning about these upstream changes, and not hindering all contributors' work.

Would be great if we could do the same thing for the Gemfile as well. While there's no single Gemfile.lock that works for every single Ruby version we test with, we could as well check in Gemfile-ruby-2.0.0.lock, ... etc as well and make use of it. (Bundler allows the Gemfile name to be controlled with BUNDLE_GEMFILE).

Nevertheless, I consider the above as an extra that we can do at any later time, and thus not in scope of this PR.

I see to reason not to merge this in ASAP, so here, take my approval.

branches:
only:
- master
jobs:
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to add an &ci_jobs id to the jobs above, and here just use jobs: *ci_jobs?

There's already quite a lot of copy/paste going on to maintain this list, would be great if we could avoid yet another thing that needs to be kept in sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the value expected in the jobs: field is an array, and there's no way to merge arrays in YAML. In other words, we can't have ci_jobs with a list of jobs but also add a few more to it (coverage, lint, deploy release) 😢

Copy link
Member

Choose a reason for hiding this comment

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

😭

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth documenting this -- e.g. in both sections, state "This is exactly the same thing as [below/above], but with the [foo/bar/baz] sections [added/removed]", so it's harder to forget to update them, and to confuse why they are different.

Comment on lines +1 to +3
# This file was generated by Appraisal

source "https://rubygems.org"
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of a shame that Appraisal doesn't seem to support the new gems.rb name for Gemfiles. Because it ends in .rb, it's not a "magic" name anymore so we'd get syntax highlighting :|

Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

I think this is a good way to keep CI from being overwhelmed by public gem changes, so overall 👍 to strategy!

It does however add a crazy number of gemfiles.

  • Is there any way to reduce/auto-generate at least some of these? I understand the point of locking is to commit some specific versions, but maybe we could keep just the lock files? (If that's possible?)
  • Would it be possible to put gemfiles in subdirectories (jruby/9.2.0.0/) instead of just a prefix? The flat structure makes it a bit overwhelming to deal with almost 400 files in one directory.

Just some thoughts.

edge:
description: Use latest version of dependencies during testing
type: boolean
default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, it would be cool if there's some way to have this workflow report to Slack when the build finishes (success or failure) so we can keep an eye on it.

@marcotc
Copy link
Member Author

marcotc commented Apr 20, 2021

@delner

Is there any way to reduce/auto-generate at least some of these? I understand the point of locking is to commit some specific versions, but maybe we could keep just the lock files? (If that's possible?)

I guess it would likely be _)technically possible to only keep the lockfiles. I'm not sure if it would play well with any changes to the Gemfile, when needed.

Would it be possible to put gemfiles in subdirectories (jruby/9.2.0.0/) instead of just a prefix? The flat structure makes it a bit overwhelming to deal with almost 400 files in one directory.

I tried this at first, but Appraisal has the path ./gemfiles/*.gemfile.* hard-coded. I created a fork of Appraisal to see if I could change it, but turns out it's too complicated to make it flexible enough for it being accepted upstream, and maintaining a fork of Appraisal doesn't seen too manageable.

Overall, I see the gemfiles folder similar to a vendor folder: you don't review everything that's in there, but you want to make sure that new changes after the initial commit are sane.

This is hopefully the last commit to have 400 files changed: all future changes should only affect a handful of lines across a subset of these files.

@marcotc
Copy link
Member Author

marcotc commented Apr 21, 2021

Ruby 2.6 build is failing because of some misconfiguration bundle platform. I'll dig into it.

@@ -61,7 +60,6 @@ services:
volumes:
- .:/app
- bundle-2.1:/usr/local/bundle
- gemfiles-2.1:/app/gemfiles
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some more details on this change? I'm not entirely sure why this needs to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be removed because - gemfiles-2.1:/app/gemfiles gives each container its own gemfiles, because before we didn't check it into the repo, and each container generated its own set of gemfiles.

Now that it's checked in, we want gemfiles to be shared across all containers, which is done indirectly by - .:/app a few lines above.

@marcotc marcotc marked this pull request as ready for review April 21, 2021 18:59
@marcotc marcotc requested a review from a team April 21, 2021 18:59
@marcotc
Copy link
Member Author

marcotc commented Apr 21, 2021

@delner I'm going to merge this with @ivoanjo's blessing, as this will stabilized our builds across the board.

This is not a perfect solution and I'm more than happy to make any changes we see fit to improve it going forward.

In it's current state, it addresses a longstanding needed we've had for CI stability.

I'll schedule the work on daily CI alerts and improvements to the top level Gemfile (which is shared by all versions).

@marcotc marcotc merged commit e3c0833 into master Apr 21, 2021
@marcotc marcotc deleted the stable-appraisal branch April 21, 2021 19:01
@github-actions github-actions bot added this to the 0.49.0 milestone Apr 21, 2021
ivoanjo added a commit that referenced this pull request May 13, 2022
We started checking in the `gemfiles/` folder in #1470, with the goal
of making our CI pipeline more stable -- e.g. so that PRs didn't
pick up new upstream releases that made them break for unrelated
reasons.

A big downside is that whenever we make even the tiniest of changes
in the `Appraisals` file, here comes huge diff of the same change
being applied dozens of times to the multiple gemfile variants.

This got me thinking: the part that guarantees CI stability is
having the `gemfile.lock` files checked in.
Could we get by without having the `.gemfile` files checked in, and
always regenerating them when needed?

Turns out we can! So here's a PR for it.

Upsides:
* A lot less files in the tree, and a lot less diff noise when
  adding new Ruby releases, gems, etc.

Downsides:
* You need to run `appraisal generate` locally to regenerate the
  `.gemfile`s. (This was already the case before #1470)
 This takes less than a second on my machine.
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.

3 participants