-
Notifications
You must be signed in to change notification settings - Fork 14
fix: pre-hotreload cartridge support #341
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
fix: pre-hotreload cartridge support #341
Conversation
ce6c37d
to
c288f08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patch! I left minor comments, but it's not critical.
test/integration/reload_test.lua
Outdated
@@ -99,6 +102,9 @@ function g.test_router() | |||
end | |||
|
|||
function g.test_storage() | |||
t.skip_if(pcall(require, 'cartridge.hotreload') == false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see, there is a check (pcall(require, 'cartridge.hotreload') == false
) of hotreload
support in several places in the code. Maybe it should be a separate function? BTW it is unlikely the realization of getting hotreload
will change, but I would put
function get_cartridge_hotreload()
return pcall(require, 'cartridge.hotreload')
end
in a separate function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, reworked
`crud` module is cartridge-independent in nature, but provides cartridge roles which are the most popular way to setup the module. The roles also not use any modern cartridge features and should work with any cartridge version. But since crud.cfg was introduced [1], it was required to add some code for roles reload [2] proper support. Now cartridge.hotreload module is unconditionally required, so roles cannot be used with cartridge older than 2.4.0. This patch fixes the behavior. 1. 6da4f56 2. tarantool/cartridge@941952e Follows #244
Before this patch, tests were marked with xfail since there was a bug in metrics module [1]. This bug is fixes in newer versions, so xfail is replaced with skip based on metrics version. 1. tarantool/metrics#334 Follows #244
c288f08
to
50865c3
Compare
Test with newer metrics and older cartridges to check previous patches.
50865c3
to
8fdd6d8
Compare
Overview This release introduces new API to check module version in code, as well as several compatibility bugfixes. New features * Add versioning support (PR #342). Bugfixes * Fix pre-hotreload `cartridge` support (older than 2.4.0) (PR #341). * Fix Tarantool version-dependent features for 3.x (PR #344).
Overview This release introduces new API to check module version in code, as well as several compatibility bugfixes. New features * Add versioning support (PR #342). Bugfixes * Fix pre-hotreload `cartridge` support (older than 2.4.0) (PR #341). * Fix Tarantool version-dependent features for 3.x (PR #344).
This PR fixes an unreasonable version support drop introduced in #244, as well as test pipeline update. See commit messages for more info.
I didn't forget about
Closes #244