ci: test against a compatible connection_pool version #706
Conversation
grzuy
changed the title
santib
force-pushed
the
fix-ci-connection-pool
branch
3 times, most recently
from
b8d489f to
b9cfa6a
Compare
santib
force-pushed
the
fix-ci-connection-pool
branch
from
b9cfa6a to
217c129
Compare
Appraisals
Outdated
| appraise "redis_store" do | ||
| # Direct version requirement on connection_pool | ||
| # can be removed once https://github.com/rails/rails#56291 is fixed and released | ||
| gem "connection_pool", "~> 2.5" |
There was a problem hiding this comment.
redis_4, redis_5, and redis_store gemfiles need it but it isn't because of redis, it's because of activesupport being installed as a dev dependency which seems unfortunate, but when trying to remove it lot of tests failed so I think we can work on that separately if we ever want to cc @grzuy
There was a problem hiding this comment.
Gotcha.
And we can't solve it by listing connection_pool 2.5 as an additional dev dependency in the gemspec, because that would affect tests that try testing when connection_pool is not present, right?
There was a problem hiding this comment.
oh I see.. I guess you are referring to this commit .. I guess that behavior got "ignored" since activesupport was added directly as a dev dependency on the gemspec, and therefore connection_pool available for all gemfiles.. 😅
At this point, since activesupport was added, the "pooled" versions of the gemfiles aren't adding any value since they are all "pooled", so I added a commit just cleaning up the spec gemfiles. Or do you think there is value in going back to testing "pooled" and "not pooled" versions, and we should go in the opposite direction removing activesupport dev dependency and such?
EDIT:
In summary I think we have many different options:
- Just add
connection_poolon the Appraisals to get the CI green - Add
connection_poolin the gemspec given thatactivesupportis already there + remove duplicated gemfiles (pooled) - Remove
activesupportfrom the gemspec + Addconnection_poolon the Appraisals
There was a problem hiding this comment.
Oh I see, didn't realize connection_pool was a direct dependency of activesupport.
At this point, since activesupport was added, the "pooled" versions of the gemfiles aren't adding any value since they are all "pooled"
Right, that seems to be true except for activesupport 7.0 which didn't depend on connection_pool yet and we're testing in the matrix. Anyway, no real value in making an exception for that "old" version given it will be become unsupported in not too long.
In summary I think we have many different options:
1. Just add `connection_pool` on the Appraisals to get the CI green 2. Add `connection_pool` in the gemspec given that `activesupport` is already there + remove duplicated gemfiles (pooled) 3. Remove `activesupport` from the gemspec + Add `connection_pool` on the Appraisals
For this PR and the goal of making main green again I am OK/I like option 2 which you have implemented right now.
As a follow up, it might be worth exploring, at least in draft PR, what happens if we remove all dev dependecies...
There was a problem hiding this comment.
Should we do something about the specs that are testing pooled vs not pooled mem cache store and redis cache store?
Are they actually duplicated?
I am OK with doing that as a follow up. No blocker.
| end | ||
|
|
||
| s.add_development_dependency "activesupport" | ||
| s.add_development_dependency "connection_pool", "~> 2.5" |
There was a problem hiding this comment.
Should we add the comment about when to remove this in the future?
Appraisals
Outdated
| appraise "redis_store" do | ||
| # Direct version requirement on connection_pool | ||
| # can be removed once https://github.com/rails/rails#56291 is fixed and released | ||
| gem "connection_pool", "~> 2.5" |
There was a problem hiding this comment.
Oh I see, didn't realize connection_pool was a direct dependency of activesupport.
At this point, since activesupport was added, the "pooled" versions of the gemfiles aren't adding any value since they are all "pooled"
Right, that seems to be true except for activesupport 7.0 which didn't depend on connection_pool yet and we're testing in the matrix. Anyway, no real value in making an exception for that "old" version given it will be become unsupported in not too long.
In summary I think we have many different options:
1. Just add `connection_pool` on the Appraisals to get the CI green 2. Add `connection_pool` in the gemspec given that `activesupport` is already there + remove duplicated gemfiles (pooled) 3. Remove `activesupport` from the gemspec + Add `connection_pool` on the Appraisals
For this PR and the goal of making main green again I am OK/I like option 2 which you have implemented right now.
As a follow up, it might be worth exploring, at least in draft PR, what happens if we remove all dev dependecies...
Appraisals
Outdated
| appraise "redis_store" do | ||
| # Direct version requirement on connection_pool | ||
| # can be removed once https://github.com/rails/rails#56291 is fixed and released | ||
| gem "connection_pool", "~> 2.5" |
There was a problem hiding this comment.
Should we do something about the specs that are testing pooled vs not pooled mem cache store and redis cache store?
Are they actually duplicated?
I am OK with doing that as a follow up. No blocker.
2 participants
CI started failing since
connection_pool3 was released https://github.com/rack/rack-attack/actions/runs/20482828723It's an indirect dependency, but our dependencies weren't prepared for these breaking changes on
connection_pool