search

PrestaShop / ADR

Architecture Decision Records for the PrestaShop project
10 stars 15 forks source link

0010 - Module Versions bump convention following PS core min version #14

Closed matks closed 3 years ago

matks commented 3 years ago

I think it’s confusing that first number doesn’t mean what it supposed to mean, I’m from the school where if you bump major version there are some breaking changes, if you bump minor version there are some new features and improvements, and of course path versions means some bugfixes etc.

Isn't it exactly the definition of SemVer 😅 ?

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and
PATCH version when you make backwards compatible bug fixes.
matks commented 3 years ago

@jolelievre To clarify: when I mention PS Core minor or major updates, I'm talking about the numbers after the 1.

So 1.7.3.4 is 7.3.4 for me. I ignore the 1 which has no meaning.

So if I mention a minor bump, it's like 1.7.5 to 1.7.6 for me

matks commented 3 years ago

@jolelievre @kpodemski @PierreRambaud ADR updated to be more clear, and added a candidate suggested by @PierreRambaud https://github.com/PrestaShop/ADR/pull/14/commits/a82854dd8b16e4bd4b943af6c692f12e3c671aee

kpodemski commented 3 years ago

@matks

@PierreRambaud suggests to stop using SemVer for modules and modify upon release the version number according to the type of changes introduced: bug fixes (patch), or new features (minor), or important changes (major).

So if I'm this understanding this correctly, if we release v1.7.8, and... we have somemodule v1.0.0... and...

yes?

matks commented 3 years ago

@kpodemski

If we exclude the "new module version bumps minimum PS version" usecases, @PierreRambaud is even simpler:

@PierreRambaud suggests to completely forget about BC breaks for modules because they are not useful for modules. Just use "patch", "minor" and "major" to refer to the size/impact of the changes. So improvements with BC breaks could be done in a minor version.

matks commented 3 years ago

@PrestaShop/prestashop-maintainers Can we host a vote or do we wait for January?

PierreRambaud commented 3 years ago

@PrestaShop/prestashop-maintainers Can we host a vote or do we wait for January?

Since everyone is on vacation, we can wait for January :sweat_smile:

eternoendless commented 3 years ago

IMO the minor/major question is mostly about compatibility with PrestaShop so that we can update modules in patch versions.

PrestaShop version 1.7.7.0 imports somemodule@^2.0. This allows any minor and patch versions of this module to be imported in subsequent patch releases of 1.7.7.x. If the module needs to increase its minimum compatible PrestaShop version, then it will require a major version release to avoid it being imported by composer into a PrestaShop version that won't support it.

matks commented 3 years ago

IMO the minor/major question is mostly about compatibility with PrestaShop so that we can update modules in patch versions.

PrestaShop version 1.7.7.0 imports somemodule@^2.0. This allows any minor and patch versions of this module to be imported in subsequent patch releases of 1.7.7.x. If the module needs to increase its minimum compatible PrestaShop version, then it will require a major version release to avoid it being imported by composer into a PrestaShop version that won't support it.

So if I get it you are in favor of my initial architecture change suggestion? 😉 (before the change suggested by @PierreRambaud )

eternoendless commented 3 years ago

I'm in favor of this (and only this) decision:

WHEN a new module version requires to bump the PrestaShop minimum version,

IF the bump is minor or major (ex: PS 1.7.6.2 => 1.7.7.0) THEN the module version must be bumped to at least the next major version (ex: ps_searchbar 2.0.1 => 3.0.0)

This part has never happened before AFAIK:

IF the core bump is a patch bump (ex: PS 1.7.6.2 => 1.7.6.4) THEN the module version must be bumped to at least the next minor version (ex: ps_searchbar 2.0.1 => 2.2.0)

Let's keep the consensual part only to move the decision forward.

BTW I have the feeling that this ADR is too argumentative. I think ADRs should be simple and objective for readability's sake: describe the problem (context) -> record decision, that's all. Keep it objective, leave the use of "I", opinions and discussion for review / issue.

tswfi commented 3 years ago

as a side note, modules also have a ps_versions_compliancy for telling which versions it works, but even core modules do not respect it but default for the max_version being the currently installed version: https://github.com/PrestaShop/ps_checkpayment/blob/dev/ps_checkpayment.php#L67

matks commented 3 years ago

ADR process is onerous to follow and I will not have the time to bring this ADR to approval point. I close it as the topic was not a critical aspect.

If anybody is interested in the topic, feel free to use my work to submit a contribution.