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

Drop legacy dependencies support #77

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Aug 26, 2024

Hello, please review this approach to build the text matrix

Drop support for:


Succesfull run: https://github.com/tagliala/rgeo-activerecord/actions/runs/10560520776


  • Test against latest stable Ruby versions
  • Test against AR 7.1 and 7.2
  • Test against jruby-9.4.8.0 and jruby-head
  • Mark edge/head tests as experimental to allow failures
  • Update checkout actions
  • Add explicit permissions for improved security

Ref: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

Copy link
Member

@keithdoggett keithdoggett left a comment

Choose a reason for hiding this comment

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

Thanks @tagliala this seems good to me. Only note may be to add AR 7.2 to the matrix as well.

@tagliala tagliala force-pushed the chore/fix-ci branch 3 times, most recently from 9ad2819 to c38d254 Compare September 3, 2024 14:24
@tagliala tagliala marked this pull request as draft September 3, 2024 14:25
@tagliala tagliala marked this pull request as ready for review September 3, 2024 14:34
@tagliala
Copy link
Contributor Author

tagliala commented Sep 3, 2024

Done

- Test against latest stable Ruby versions
- Test against AR 7.1 and 7.2
- Sort by AR version descending
- Do not test incompatible Ruby / AR versions (ex: Ruby 3.0 / AR 5.x)
- Use `include` syntax for legacy AR versions
- Mark `edge` tests as `experimental` to allow failures
- Update checkout actions
- Add explicit permissions for improved security
- Fix test warning related to keyword argument in Struct#initialize

Ref: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions
@BuonOmo
Copy link
Member

BuonOmo commented Sep 3, 2024

I'd also remove unmaintained ruby and activerecord versions (which ultimately might let us keep the original matrix?). WDYT?

@tagliala
Copy link
Contributor Author

tagliala commented Sep 3, 2024

I'd also remove unmaintained ruby and activerecord versions (which ultimately might let us keep the original matrix?). WDYT?

You mean just remove from the matrix or drop support?

@BuonOmo
Copy link
Member

BuonOmo commented Sep 3, 2024

drop support. But what do you have in mind more than removing it from the matrix ?

@tagliala
Copy link
Contributor Author

tagliala commented Sep 4, 2024

But what do you have in mind more than removing it from the matrix ?

Change this:

spec.required_ruby_version = ">= 2.5.0"

Then I usually search documentation and source code because there may be references, conditional checks, or feature detection related to unsupported rails / ruby

Ref (just the latest thing I've worked on): jamesmartin/inline_svg#164

@tagliala
Copy link
Contributor Author

tagliala commented Sep 5, 2024

Since 6.1 is going in EOL at the end of the month, I would go for

  • Ruby >= 3.1
  • Rails >= 7.0
  • RGeo >= 3 (are 1.x/2.x considered legacy?)

@tagliala tagliala marked this pull request as draft September 5, 2024 09:48
@tagliala tagliala changed the title Improve CI Drop legacy dependencies support Sep 5, 2024
As per EOL policies, remove support for:
- Ruby < 3.1
- Rails < 7.0

Also:
- Remove RGeo < 3 support
- Test against jruby 9.4.8 and jruby-head
Copy link
Member

@keithdoggett keithdoggett left a comment

Choose a reason for hiding this comment

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

@tagliala looks good! Thank you. My only question is for @BuonOmo about the version and how to sequence releases.

VERSION = "7.0.1"
VERSION = "8.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

@BuonOmo will we have to synchronize this with the postgis adapter PR that you have up? Do we need a major version bump here?

Copy link
Member

Choose a reason for hiding this comment

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

well since there is no change in the codebase, I don't think we have to do anything. TBH I would even consider not bumping major.

Comment on lines +32 to 34
Version `8.0+` supports ActiveRecord 7.x with `rgeo` 3.0+

Version `7.0+` supports ActiveRecord 5.x, 6.x, and 7.x with `rgeo` 1.0+
Copy link
Contributor Author

Choose a reason for hiding this comment

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

major version bumped because of this approach. I can bump minor and change 8.0+ to 7.1+

Copy link
Member

@BuonOmo BuonOmo left a comment

Choose a reason for hiding this comment

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

Version is not that important to me, I'll let you guys decide !

BTW @tagliala, is there any reason why the PR is still a draft?

@tagliala tagliala marked this pull request as ready for review September 9, 2024 15:40
@tagliala
Copy link
Contributor Author

tagliala commented Sep 9, 2024

Status changed back to Open. This PR will close #75

@keithdoggett
Copy link
Member

The only thing with the version I'm thinking that might be breaking is that we're enforcing rgeo 3. I'm not sure about best practices for when there's a big dependency upgrade like that, but since this gem is usually just used downstream of an adapter and not independently installed a major change is ok since the postgis adapter is about to undergo a major change too.

@keithdoggett keithdoggett merged commit 379bb45 into rgeo:master Sep 11, 2024
16 checks passed
@keithdoggett
Copy link
Member

Thanks @tagliala

@tagliala
Copy link
Contributor Author

Welcome. I didn't check if there is dead code, now that 2.0 and 1.0 have been dropped. I'm not familiar with RGeo internals

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.

Let drop support for rgeo prior 3.0
4 participants