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

Use Appraisal in CI #1230

Closed
wants to merge 19 commits into from
Closed

Use Appraisal in CI #1230

wants to merge 19 commits into from

Conversation

Spone
Copy link
Collaborator

@Spone Spone commented Jan 6, 2022

Related to #1227 (comment)

In my initial setup of Appraisal (#1225), I forgot to specify which gemfile to use in the CI configuration.

@Spone Spone requested a review from boardfish January 6, 2022 10:38
@Spone Spone self-assigned this Jan 6, 2022
@@ -46,7 +49,7 @@ jobs:
- uses: actions/cache@v2
with:
path: vendor/bundle
key: gems-build-rails-${{ matrix.rails_version }}-ruby-${{ matrix.ruby_version }}-${{ hashFiles('**/Gemfile.lock') }}
key: gems-build-rails-${{ matrix.rails_version }}-ruby-${{ matrix.ruby_version }}-${{ hashFiles('**/gemfiles/rails_${{ matrix.rails_version }}.gemfile.lock') }}
Copy link
Collaborator Author

@Spone Spone Jan 6, 2022

Choose a reason for hiding this comment

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

I'm not sure this works as expected. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this part probably works alright. I'm inclined to believe it's setting BUNDLE_GEMFILE that isn't doing anything.

if caching was failing but the Gemfile was fine, GitHub Actions would try to install turbo-rails. I'm inclined to believe setting the Gemfile doesn't work, but caching still does, because there's no mention of turbo-rails or tailwindcss-rails in the logs, and the logs still say they're using a gem that has already been downloaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tailwindcss-rails now appear in the logs of the latest runs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like the call to hashFiles is returning anything.

@boardfish
Copy link
Collaborator

Might it make sense to use Appraisal directly the same way it's mentioned inCONTRIBUTING.md, or does that complicate caching?

@Spone
Copy link
Collaborator Author

Spone commented Jan 6, 2022

Might it make sense to use Appraisal directly the same way it's mentioned inCONTRIBUTING.md, or does that complicate caching?

Something like that?

- bundle exec rake
+ bundle exec appraisal rails-${{ matrix.rails_version }} rake

@boardfish
Copy link
Collaborator

Yeah. Come to think of it, I don't think it would cause caching issues, since it'd be installing the gems to the same place. You're right to be setting the cache keys based on the Rails version in use, though.

@Spone
Copy link
Collaborator Author

Spone commented Feb 11, 2022

What do you think of this approach @joelhawksley & @BlakeWilliams?

@Spone
Copy link
Collaborator Author

Spone commented Feb 14, 2022

I finally got CI to pass, with appraisal running the specs. It's currently running a fork of Appraisal, with this PR thoughtbot/appraisal#174 merged. So we should probably wait for the new version of Appraisal before going forward with this.

@@ -8,6 +8,10 @@ rails_version = "#{ENV['RAILS_VERSION'] || 'main'}"
gem "capybara", "~> 3"
gem "rails", rails_version == "main" ? { git: "https://github.com/rails/rails", ref: "main" } : rails_version

group :development, :test do
gem "appraisal", github: "excid3/appraisal", branch: "fix-bundle-env"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we shouldn't be using thoughtbot/appraisal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes because the fix we need is not yet merged: thoughtbot/appraisal#174

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can keep tabs on where @excid3's fork is at to ensure it's not susceptible to upstream issues with Appraisal?

Copy link

Choose a reason for hiding this comment

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

Just FYI, I am skipping appraisal in CI right now and just setting BUNDLE_GEMFILE via matrix.

https://github.com/excid3/noticed/blob/master/.github/workflows/ci.yml#L28

Copy link
Collaborator

@boardfish boardfish left a comment

Choose a reason for hiding this comment

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

The call to hashFiles when setting the cache key isn't returning anything, so the cache can be overwritten by PRs.

@@ -46,7 +49,7 @@ jobs:
- uses: actions/cache@v2
with:
path: vendor/bundle
key: gems-build-rails-${{ matrix.rails_version }}-ruby-${{ matrix.ruby_version }}-${{ hashFiles('**/Gemfile.lock') }}
key: gems-build-rails-${{ matrix.rails_version }}-ruby-${{ matrix.ruby_version }}-${{ hashFiles('**/gemfiles/rails_${{ matrix.rails_version }}.gemfile.lock') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like the call to hashFiles is returning anything.

@Spone
Copy link
Collaborator Author

Spone commented Feb 15, 2022

The call to hashFiles when setting the cache key isn't returning anything, so the cache can be overwritten by PRs.

Hmmm... I think that we cannot hash the gemfiles/rails_****.gemfile.lock files because they are not committed, so they do not exist when the actions/cache@v2 action runs.

@boardfish
Copy link
Collaborator

I think it might be sufficient to use the Gemfiles themselves under gemfiles.

@boardfish
Copy link
Collaborator

Seems like the multiple calls to the cache action might be interfering with each other here.

@boardfish
Copy link
Collaborator

What needs doing to move this forward? Is there any way I can help? From what I can see it's probably just the gem caching that remains to be solved, but I'm not sure if you had any other plans @Spone.

@Spone
Copy link
Collaborator Author

Spone commented Mar 14, 2022

What needs doing to move this forward? Is there any way I can help? From what I can see it's probably just the gem caching that remains to be solved, but I'm not sure if you had any other plans @Spone.

Yes I think the last blocking item is fixing the caching issue. If you have time to look into it that would be awesome!

@Spone
Copy link
Collaborator Author

Spone commented Mar 16, 2022

I'll close this, as it's superseeded by #1308.

@Spone Spone closed this Mar 16, 2022
@Spone Spone deleted the fix-appraisal-ci branch March 16, 2022 11:26
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.

4 participants