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

fix: pre-hotreload cartridge support #341

Merged

Conversation

DifferentialOrange
Copy link
Member

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

  • Tests
  • Changelog
  • Documentation (not needed)

Closes #244

@DifferentialOrange DifferentialOrange marked this pull request as draft February 3, 2023 15:17
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/support-older-cartridges branch 4 times, most recently from ce6c37d to c288f08 Compare February 6, 2023 08:11
@DifferentialOrange DifferentialOrange marked this pull request as ready for review February 6, 2023 08:47
Copy link
Contributor

@AnaNek AnaNek left a 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.

@@ -99,6 +102,9 @@ function g.test_router()
end

function g.test_storage()
t.skip_if(pcall(require, 'cartridge.hotreload') == false,
Copy link
Contributor

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.

Copy link
Member Author

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
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/support-older-cartridges branch from c288f08 to 50865c3 Compare February 13, 2023 11:54
Test with newer metrics and older cartridges to check previous patches.
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/support-older-cartridges branch from 50865c3 to 8fdd6d8 Compare February 13, 2023 12:01
@DifferentialOrange DifferentialOrange merged commit 6f041ee into master Feb 13, 2023
@DifferentialOrange DifferentialOrange deleted the DifferentialOrange/support-older-cartridges branch February 13, 2023 12:54
DifferentialOrange added a commit that referenced this pull request Mar 13, 2023
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).
DifferentialOrange added a commit that referenced this pull request Mar 13, 2023
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).
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.

2 participants