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

Clean up Appraisal and CI config; add Rails 7.2 and Ruby 3.3 #723

Merged
merged 12 commits into from
Aug 13, 2024

Conversation

mattbrictson
Copy link
Contributor

@mattbrictson mattbrictson commented Aug 11, 2024

Nearly all CI builds have been failing recently for various reasons. This PR fixes those failures and cleans up cruft in the Appraisal and CI config. Namely:

  • An unquoted 3.0 Ruby version number in YAML is interpreted as 3, and was installing the latest 3 version, 3.3.4, rather than a 3.0 version, as intended. I quoted all Ruby version numbers in the ci.yml file to avoid this problem.
  • The Appraisal and CI configs were targeting "Rails 8.0", but no such version of the Rails gem exists. The latest published version is 7.2.0. The main branch of Rails is targeting 8.0, but no prerelease has been published yet. I removed the references to Rails 8.0 for now. It can be added back in once an 8.0 prerelease becomes available.
  • The Appraisal config still referenced Rails 5.0 and 5.1, which audited no longer supports. I removed them.
  • The Rails 7.0 and 7.1 Appraisal configs had an incorrect gem requirement for sqlite3 that was causing these builds to completely fail. I corrected the requirement.
  • Many builds were failing due to code coverage metrics. I increased the allowed number of uncovered lines so that CI will pass.

In addition to the cleanup, I expanded the CI matrix:

  • Added Rails 7.2
  • Added Ruby 3.3

@mattbrictson mattbrictson marked this pull request as draft August 11, 2024 00:43
@mattbrictson
Copy link
Contributor Author

CI is completely passing in my fork, so I think this PR is ready to go. https://github.com/mattbrictson/audited/actions/runs/10335743861

@mattbrictson mattbrictson marked this pull request as ready for review August 11, 2024 00:57
@gjtorikian
Copy link

gjtorikian commented Aug 12, 2024

Thank you for the operator (>=) addition. 🙏

EDIT:

gem "audited", github: "mattbrictson/audited", ref: "ci-cleanup-rails-7.2"

for the searchers.

@danielmorrison danielmorrison merged commit 0d18048 into collectiveidea:main Aug 13, 2024
76 checks passed
@@ -16,11 +16,11 @@ Gem::Specification.new do |gem|

gem.required_ruby_version = ">= 2.3.0"

gem.add_dependency "activerecord", ">= 5.2", "< 8.1"
gem.add_dependency "activesupport", ">= 5.2", "< 8.1"
gem.add_dependency "activerecord", ">= 5.2", "< 8.0"
Copy link

Choose a reason for hiding this comment

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

nooo rip. that means when rails 8.0 is out i will have to fork this project again to be able to use rails main branch. i thought i had more time 😭

honestly i don't know why there is a restriction on the upper bound
it should be like this gem.add_dependency "activerecord", ">= 5.2"

Copy link
Member

Choose a reason for hiding this comment

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

I don't really have a strong opinion here. I see this pattern of limiting the upper bound a lot, but is it really helping more than hurting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal for this PR was to get CI passing and confirm Rails 7.2 compatibility. I removed the rails80 appraisal because it was not working. Without a passing test suite for 8.0, I didn't feel comfortable allowing 8.0 in the gemspec, and I don't have a real app running on Rails main that I could manually test.

Rubygems itself frowns upon unbounded dependencies. I don't know how closely their recommendations are followed, but the Rubygems validation code emits a warning for it:

https://github.com/rubygems/rubygems/blob/b22914018e9c5f7b5b31eab6fa39ea4e82c623dc/lib/rubygems/specification_policy.rb#L222-L235

For audited specifically, this is a mission-critical gem, it can result in data loss if it doesn't work correctly, and it hooks deeply into ActiveRecord. So I would be uncomfortable if it allowed versions of Rails that were not thoroughly tested. So I lean toward being more strict in the gemspec.

That said, ultimately if this is a very-lightly maintained gem, and the philosophy is "use it at your own risk", then an unbounded requirement makes sense. On the other hand, if there is a plan for actively maintaining and testing every release of Rails and Ruby versions going forward, including prereleases, then I think a more strict requirement would work.

end

appraise "rails80" do
Copy link

Choose a reason for hiding this comment

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

so in the changelog it says "Add rails 8 support" and now we remove the appraise rails80 ?

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