Ensuring LHM supports ActiveRecords version >= 6.1 #120
Conversation
EtienneBerubeShopify
requested review from
Manan007224,
coding-chimp and
shuhaowu
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
3 times, most recently
from
0165a97 to
a49d76f
Compare
| it 'logs and creates client' do | ||
| active_record_config = {username: 'user', password: 'pw', database: 'db'} | ||
| ActiveRecord::Base.stubs(:connection_pool).returns(stub(spec: stub(config: active_record_config))) | ||
| ActiveRecord::Base.stubs(:connection_pool).returns(stub(db_config: stub(configuration_hash: active_record_config))) |
There was a problem hiding this comment.
This is OK since the test environment uses ~>6.1.4
lhm.gemspec
Outdated
| s.add_development_dependency 'mocha' | ||
| s.add_development_dependency 'rake' | ||
| s.add_development_dependency 'activerecord' | ||
| s.add_development_dependency 'activerecord', '~> 6.1.4' |
There was a problem hiding this comment.
I'm a bit confused. Doesn't LHM depend on an ActiveRecord connection? Why is it only a development dependency? I guess this is more a general question and not specific to this PR.
There was a problem hiding this comment.
Yes, it does require acrtiverecord to use. However, for development we would require 6.1.4 and greater to ensure some consistency with the development (which has been a problem in the past)
There was a problem hiding this comment.
Couldn't we just check in the Gemfile.lock? I'm not sure why we put it in gitignore in the first place. The commit is just saying "No gemfile.lock for gems". I checked a few other Shopify gems and they all have it checked in. 🤔
There was a problem hiding this comment.
At one point LHM didn't depend on AR... or at least that was the intention. Not sure if that was ever successful.
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
6 times, most recently
from
251b29a to
3e35eef
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
10 times, most recently
from
9421698 to
885a0f6
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
from
885a0f6 to
3cd603b
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
from
3cd603b to
f5b50a4
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
from
f5b50a4 to
b801123
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
7 times, most recently
from
de8e715 to
b0eb65d
Compare
EtienneBerubeShopify
force-pushed
the
etienne-lhm-active-record-backwards-compatible
branch
from
b0eb65d to
ecf7a7a
Compare
coding-chimp
deleted the
etienne-lhm-active-record-backwards-compatible
branch
3 participants
Currently, LHM's Slave lag throttler can only be used with version of ActiveRecords that are LESS than 6.1. The problem stems from this line:
ActiveRecord::Base.connection_pool.spec.config.dupWhich was changed in version 6.1.0 to
ActiveRecord::Base.connection_pool.db_config.configuration_hash.dupIn order to not break any users downstream or induce any backwards compatibility issues, LHM should be able to differentiate between both versions and call the right function accordingly.