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

Implement custom preloads - backport to 4-x-stable #132

Closed

Conversation

apauly
Copy link
Contributor

@apauly apauly commented Nov 5, 2023

This PR backports the recently added "custom preloads" feature to the 4-x-stable branch.

In addition to a cherry pick, it required a couple of other updates:

  • Adjust circle ci config as the docker images references on that branch are not available anymore, so I started from the current master and just replaced the workflows to match the old branch
  • Use older sqlite version for ruby 2.6
  • avoid using GlobalID#locate_many during specs

@apauly apauly requested a review from jturkel as a code owner November 5, 2023 19:21
Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

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

Thanks for resurrecting the broken builds on this branch. A few minor comments on the build setup but the core of the backport looks good.

@@ -2,18 +2,18 @@ version: 2.1
jobs:
lint:
docker:
- image: salsify/ruby_ci:2.6.10
- image: ruby:3.0.6
Copy link
Member

Choose a reason for hiding this comment

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

We lint against the oldest supported version of Ruby. Can you get the lint job working against ruby:2.6.10?

@@ -2,18 +2,18 @@ version: 2.1
jobs:
lint:
docker:
- image: salsify/ruby_ci:2.6.10
- image: ruby:3.0.6
working_directory: ~/goldiloader
steps:
- checkout
- run:
Copy link
Member

Choose a reason for hiding this comment

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

Can this install of SQLite be removed if we're pinning the gem to a corresponding older version?

@@ -6,4 +6,6 @@ gem "activerecord", "5.2.8.1"
gem "activesupport", "5.2.8.1"
gem "rails", "5.2.8.1"

gem "sqlite3", "1.5.4" if RUBY_VERSION < "2.7"
Copy link
Member

Choose a reason for hiding this comment

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

The gemfiles/*.gemfile files are generated by appraisals. Can you update the Appraisals file so these gemfiles can be regenerated via appraisal generate?

Copy link
Contributor Author

@apauly apauly Nov 9, 2023

Choose a reason for hiding this comment

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

I tried to do that with the install_if option, but that failed for older ruby versions.
Can you give me a hint on where appraisal generate is actually being evaluated? It seems this is not during the CI build but locally before committing, so the conditional checking for the RUBY_VERSION cannot go into the Appraisals file, right? Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Correct we run appraisal generate locally and and checkin the generated gemfiles whenever we modify Appraisals. Leveraging the install_if option in Appraisals looks like it should work.

@apauly
Copy link
Contributor Author

apauly commented Dec 8, 2023

Hi @jturkel,
thanks again for your feedback!

Unfortunately I didn't find the time yet to continue working on this.
As my/our need for a backport of this feature was fixed since the initial PR, I will probably not be able to find the time continue working on this.

I'd be cool if the changes might be helpful when somebody else is looking for a backport, but for now, I'm closing the PR.

@apauly apauly closed this Dec 8, 2023
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