Add "Must bump version" check

[dreispt]

This is a suggestion for a new check:

When doing a PR, it should bump the module's version should be bumped, either if we are porting, adding feature, or fixing code.
The check should run only for PRs.

Special case: some PR might not involve modules (e.g: changir .travis.yml) - in these case the check should not fail.

[max3903]

👍

[moylop260]

Its is posible if you add version in required keys from configuration file of PR from mqt

https://github.com/OCA/maintainer-quality-tools/blob/master/travis/cfg/travis_run_pylint_pr.cfg#L14

[moylop260]

Or are you talking about auto increase the version number to manifest file?

Like as a plugin to odoo modules of https://pypi.python.org/pypi/bumpversion/0.5.3?

[sebastienbeau]

👍 for adding the check.
bumpversion can be a good tool for incrementing the version

[dreispt]

The point is not to autoincrement (that depends on the type of change made).
The point is to check that we don't forget to change the version number when doing a PR.
You don't even have to check if it increases, it's enough to check if it changed.

[moylop260]

@dreispt
Thanks for clarify
I got it

I'm agree with you

What about if 2 people are working in 2 different changes?
The PRs will tend to diverge with conflicts because change the same line.
If PR1 is merged the PR2 will show conflicts or viceversa.
And the reviewers will request to contributors that fix the conflict but this could be requested many times by each PR merged of the same module.

Generally in many projects the change of versions are controlled from stable branch to avoid this issues.

IMHO we should think use a workflow (auto or manual) to change it from stable branch.

[elicoidal]

👍 with the check
👎 for auto-increment

[dreispt]

@moylop260 I think that's a corner case we can live with. And if two people are changing the same module, odds are that you will have conflicts and need a rebase.

[moylop260]

Could somebody help me to create this check?

If you want help use the following videos to learn
https://asciinema.org/a/26890
https://asciinema.org/a/26889

[moylop260]

Maybe this check is not a pylint-odoo one.
Because we need to extract a git diff and verify if there is a manifest version change and pylint-odoo is not using a git diff

IMHO It's a new script from MQT, since we have git_run script to know a git diff of items changed.

[sbidoul]

Actually I'm not sure bumping version at each PR is the right thing to do.

In "normal" projects the maintainer bumps the version when he does a release. Between releases there is dev, alpha and beta versions.

If we want to encourage faster merges (aka optimistic merges) we need a mechanism to mark releases that the maintainer deems stable.

So in the current state of affairs, I believe it's better to not force bumping versions in each PR and let the maintainer do it manually when he thinks the module is stable enough.

[moylop260]

+1 with @sbidoul

[dreispt]

@sbidoul AFAICT in OCA world every PR merge is a stable release.

[dreispt]

To make it clear: my suggestion is not for automatic version bumping - that should be a human action, since we must decide what digit to bump.
I'm suggesting to check that a PR always bumps (or at least changes) the module version number.

[sbidoul]

in OCA world every PR merge is a stable release.

Yes, and I think we must strive to change that.

That's why I'm 👎 on this check: encouraging people to bump version at each merge would make it harder to transition to optimistic merge + manual release.

[lasley]

I think that we will need a better tagging mechanism if we start releasing non-stable versions, with beta/alpha tags excluded from the Odoo Apps store & everyone's automatic upgrade systems. Stable module one day, broken the next - not really the way to go IMO.

👍 on the check, 👎 on auto-bump BTW

[moylop260]

Maybe we are seeing the modules like as a python package.
I showed in OCA sprint to @sbidoul How to auto bump version in python package with a release/tag after merge.
(I know here the matter is not auto-bump version)

The point is that working with python package you don't change the version in each PR.

Imagine 3 developers working in a PR in the same module.

Module	Developer	Stable version	Bumped version	Status of PR
Module A	Tom	8.0.1.0.0	8.0.1.0.1	Open
Module A	Jhon	8.0.1.0.0	8.0.1.0.1	Open
Module A	Jarry	8.0.1.0.0	8.0.1.0.1	Open

After days of feedback and more commits a PR is merged.

Module	Developer	Stable version	Bumped version	Status of PR
Module A	Tom	8.0.1.0.1	<-	Merged
Module A	Jhon	8.0.1.0.0	8.0.1.0.1	Conflicts manifest file
Module A	Jarry	8.0.1.0.0	8.0.1.0.1	Conflicts manifest file

After fix conflicts

Module	Developer	Stable version	Bumped version	Status of PR
Module A	Jhon	8.0.1.0.1	8.0.1.0.2	Open (Rebased)
Module A	Jarry	8.0.1.0.1	8.0.1.0.2	Open (Rebased)

After days of feedback and more commits a PR is merged.

Module	Developer	Stable version	Bumped version	Status of PR
Module A	Jhon	8.0.1.0.1	8.0.1.0.2	Conflicts manifest file
Module A	Jarry	8.0.1.0.2	<-	Merged

After fix conflicts (Again)

Module	Developer	Stable version	Bumped version	Status of PR
Module A	Jhon	8.0.1.0.2	8.0.1.0.3	Open (Rebased*2)

IMHO a bump version is not work of a pull request.
If a process increase the conflicts of a PR create a game that discourage new contributions.

I my case if I see a opened PR of the same module... better I will wait a merge before of create my PR today in order to avoid the game of conflicts, fix, rebase, conflicts, fix, rebase...

IMHO It should be work of the maintainer who decide what PR merge first.
But this is a work that the maintainer don't want to do.

For pylint-odoo we are using pbr and if we create a new git tag -s v8.0.1.0.2 {SHA} then a release is auto-created and a new version is auto-deployed in pypi.

We decide the version with the tag.

Other tools to bumpversion is bumpversion to auto increase the version {major.minor.patch} use the following commands:

• For increase .patch section
  • bumpversion patch --tag
• For increase .minor section
  • bumpversion minor --tag
• For increase .major section
  • bumpversion major --tag

This auto-create a commit, a tag and change the manifest.

We decide the version with the patch|minor|major command.

Maybe, we can use a mix of pbr and bumpversion to change the manifest version based in (something):

• Maybe a standard of commit message
• Maybe a tag for each module
• Maybe a mix of: auto-increase just patch sections but for minor and major use manually.

I don't know what is the better solution for 2 worlds.

[moylop260]

@lmignon What is you opinion for this?

[max3903]

If there is many PR on the same module, the second PR should be made
against the first one, not the OCA branch.

[image: Ursa Information Systems]
Maxime Chambreuil__Project Manager / Consultant

Ursa Information Systems
1450 W Guadalupe Road, Suite 132
Gilbert, Arizona, 85233

• Office: 1-855-URSA ERP x 710 1-855-877 2377 x 710
  Mobile: 1-602-427-5632*

On Wed, Oct 19, 2016 at 4:59 PM, moylop260 notifications@github.com wrote:

@lmignon https://github.com/lmignon What is you opinion for this?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#59 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5XsF59vXUMfhY4_a_AIdoSRxrh1aV1ks5q1pKzgaJpZM4JagRd
.

[moylop260]

@max3903 I dont understood you sorry
Could you help me to clear it?

[max3903]

Instead of having 3 PR opened against the OCA branch. The second and third
PR should be made against the branch of the first PR.

One example:
1st PR: OCA/server-tools#543
2nd PR: naousse/server-tools#1

[image: Ursa Information Systems]
Maxime Chambreuil__Project Manager / Consultant

Ursa Information Systems
1450 W Guadalupe Road, Suite 132
Gilbert, Arizona, 85233

• Office: 1-855-URSA ERP x 710 1-855-877 2377 x 710
  Mobile: 1-602-427-5632*

On Wed, Oct 19, 2016 at 5:11 PM, moylop260 notifications@github.com wrote:

@max3903 https://github.com/max3903 I dont understood sorry
Could you help me to clear it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#59 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5XsO_KTrIc10X_NCO6pa0IT5Dxrdcfks5q1pWpgaJpZM4JagRd
.

[sbidoul]

If there is many PR on the same module, the second PR should be made against the first one, not the OCA branch.

This is most unusual.

[max3903]

Also, if there is 3 PR on the same module, maybe an issue should have been
created in the first place... but this is another topic and I agree
unusual, but we can improve and encourage good behavior, right?

[image: Ursa Information Systems]
Maxime Chambreuil__Project Manager / Consultant

Ursa Information Systems
1450 W Guadalupe Road, Suite 132
Gilbert, Arizona, 85233

• Office: 1-855-URSA ERP x 710 1-855-877 2377 x 710
  Mobile: 1-602-427-5632*

On Wed, Oct 19, 2016 at 5:18 PM, Stéphane Bidoul (ACSONE) <
notifications@github.com> wrote:

If there is many PR on the same module, the second PR should be made
against the first one, not the OCA branch.

This is most unusual.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#59 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5XsOCwvUhoaqMWWdv7ML4ZOditIr62ks5q1pdEgaJpZM4JagRd
.

[sbidoul]

FWIW setuptools-odoo already auto-bumps dev versions in packaged addons distributions, based on the number of commits since last version change (similar to what pbr does). Have a look at https://wheelhouse.odoo-community.org/oca

[lasley]

Imagine 3 developers working in a PR in the same module.

You raise a good point. I am coming around to see the need for the version bump being outside of the PR. Here's an example of PRs submitted to the same module, all with independent purposes and no conflicts between them (from what I remember):

• Improve checkpoint connector#216
• Fix some tests connector#215
• [IMP] [9.0] Allow configuring runner from config file connector#213

None of these PRs raised the patch version in the manifest. Had we tried to do so, the respective authors would have had to watch for other authors' PRs to possibly get merged in front of their's.

Another question is the credibility of the CI after another branch for the same module has been merged. How will it know that the version has incremented from under it?

While our amazing concept of modularity does keep us from this in most cases, it does happen.

Instead of having 3 PR opened against the OCA branch. The second and third PR should be made against the branch of the first PR.

IMO this only holds true if the PRs are linked to each other in some way. If I author a bug fix, I don't expect to become responsible for a feature addition due to a PR being submitted to my branch. This could also cause lost PRs if they are being submitted to not-so-responsive repos.

[moylop260]

Using odoo.version.breaking.feature.fix version standard most of the time we are creating many changes of last digit:

• 5% odoo.version
  • Increment it manually
• 10% .breaking
  • Increment it manually
• 15% .feature
  • Increment it manually
• 70% .fix
  • Increment it automatically in stables branches like as transifex (If the version was not changed)
    • We can use the transifex travis build to add a extra commit with the bump version similar to @sbidoul script.

[lmignon]

@moylop260 I like the idea to auto increment the .fix number (if .breaking or .feature or .fix are not already incremented) on merge. IMO that should be done in a dedicated script (no transifex) and use the @sbidoul script.

[yajo]

Thanks, I'm updating #59 (comment) with votes to have all clear.

I added 5th option from #59 (comment).

@lmignon By manual do you mean option 1, 4 or 5?

[sbidoul]

@yajo if you consolidate votes in the same comment, please make sure to include all +/- arguments too.

[yajo]

I'm trying to, please tell me the +/- points you miss for any of them 😉

[sbidoul]

@yajo for instance you do not reflect my argument about staying close to usual open source processes. Also when you say "aims to change current workflow" this is not accurate. I do not want to change the current workflow now, I want to keep the door open to evolving the current workflow. This is quite different.

[yajo]

Thanks, I changed both. If you miss any other points, please tell.

[NL66278]

My vote is for option 2: autobump last position (fix), manually change others.

[lmignon]

@yajo

@lmignon By manual do you mean option 1, 4 or 5?

option 5 must stay the default and the auto bumping version must be a decision by project/PSC. Therefore the autobump can't be put by default on each project.

[simahawk]

I vote for opt 2 too 7

[pedrobaeza]

My vote diverges a bit, but it's something that has already be said: we must check in MQT that the version is changed on each PR (that is to force user to change it), but leave the user to decide which part he/she changes according the PR. Conflicts because of this won't be the usual day to day.

[simahawk]

What if opt 2 could be tied to a keyword in PR name? Like: [FIX] bla bla -> change "fix" part.

[yajo]

If possible, it would be cool. You mean trigger the linter (opt.1) if [FIX] is not in the title, trigger the autoupdate (opt.2) otherwise, right?

[simahawk]

yep, would be nice and make everybody happy... I guess :)

[yajo]

Nice, option 7 added, I switch our votes for that. 😉

[yvaucher]

For me the bump for fix and improvement is something that must be done the merging process.
While for port and big refactoring I think this could be done by the author of the PR.

However, if we can simply automate it, it would be great.

We have 4 cases to take care of:

automated (to do while merging, responsibility of the merger):

• [FIX] for x.x.y.y.+1
• [IMP] for x.x.y.+1.0

manually changed by commiter in the PR:

• Big refactor or new major version
  [REF] [other?] x.x.+1.0.0
• Portage to Odoo version +1
  [MIG] +1.0.1.0.0

Thus the check on title might not be necessary if version was manually changed.

Unless we want to force all PR title to a set of tags in which case we could extend the automated behavior to bigger changes like migration and major versions using tags in each PR title.

[yajo]

I think that could lead to problems. When we talk about a FIX, it always increases 5th number; but an IMP could increase the 4th number (if it is non-breaking) or the 3rd (if it is breaking). The responsibility to choose which one to increase in this case is for a human.

Also, the point for automatic bump is fix simultaneous PR conflicts. Yes, it could happen that 2 IMPs come at the same time for the same addon, but seems not so usual, and a rebase in that case could be required not just for the version but for the code too, so I would not lose too much time in making such a hard job that would just cover a random percent of contributions. Option 7 covers 100% of them.

BTW, having 2 MIG at the same time is out of scope; conflict is guaranteed, so nothing to do there.

In any case, I think that your suggestion could be applied in a later step (like v2.0 of the autobumper) if we go for option 7, so let me add it there with your vote.

[lasley]

Option 7 seems like an ideal scenario, but IMO we will need to align our contribution docs for that. These prefixes aren't mentioned anywhere, so it will be unlikely for anyone other than veterans to get it right.

I like to hope for ideal scenario though, so my vote is changed to 7. With my vote on this side of the fence, I'll help out with the docs if it happens 😉

[pedrobaeza]

You can take the section "Commit message" from Odoo guidelines as reference: https://www.odoo.com/documentation/8.0/reference/guidelines.html

[max3903]

@sbidoul If we go with option 2, can we package the branches being reviewed
and use 'dev' and the PR #?

[image: Ursa Information Systems]
Maxime Chambreuil__Project Manager / Consultant

Ursa Information Systems
1450 W Guadalupe Road, Suite 132
Gilbert, Arizona, 85233

• Office: 1-855-URSA ERP x 710 1-855-877 2377 x 710
  Mobile: 1-602-427-5632*

On Mon, Oct 24, 2016 at 3:48 AM, Daniel Reis notifications@github.com
wrote:

IMO two PRs targetting the same module is not such a frequent event, so
keeping the process manual is an option. If you make a PR, you are expected
to bump the version, so my proposal here is for MQT Lint to check that the
version was changed.

If someone wants to implement automatic version bumping, that's a nice to
have, and I would go for option 2, keeping the already long 5 digit version
numbers, and avoiding an even longer 6 digits versions.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#59 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5XsKcu4n5fIlCURytY6E8E_ritHNB2ks5q3HD3gaJpZM4JagRd
.

[max3903]

Can we coexist? Can we have PSC or repo where maintainers accepts PR easily
and do release management?

I would like to try to see if I get more contributions.

[image: Ursa Information Systems]
Maxime Chambreuil__Project Manager / Consultant

Ursa Information Systems
1450 W Guadalupe Road, Suite 132
Gilbert, Arizona, 85233

• Office: 1-855-URSA ERP x 710 1-855-877 2377 x 710
  Mobile: 1-602-427-5632*

On Thu, Oct 20, 2016 at 10:03 AM, Pedro M. Baeza notifications@github.com
wrote:

@sbidoul https://github.com/sbidoul, I don't agree with that change in
the process, because at the end, who will test that "beta" merge to bump
the version? I don't see any advantage in that change, and more
disadvantages and work that is not tracked the same as is done in the PRs.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#59 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5XsCykbXeKaT1s3JYHM0hSqmWOzbs1ks5q14K2gaJpZM4JagRd
.

[yajo]

If we go with option 2, can we package the branches being reviewed and use 'dev' and the PR #?

That effort goes well if you only need 1 PR to be merged in your deployment, but if that PR gets stalled or you need more than one, then you still need to do a git merge... And that happens a lot, from my experience.

Can we coexist? Can we have PSC or repo where maintainers accepts PR easily and do release management?

I wouldn't veto against that as long as stable branches are not affected. If you want to open a 8.0-unstable, 9.0-unstable, etc. branch and merge anything there, I have no problem; but the main reason we can officially support all the hundreds of OCA addons in many companies is the reliable stability filters that code passes before reaching stable.

Of course, I hope you agree that merges from i.e. 9.0-unstable to 9.0 should go through a PR.

But we are merging 2 issues here. One is the optimistic merge proposal, and other is the autobump one.

[sbidoul]

But we are merging 2 issues here. One is the optimistic merge proposal, and other is the autobump one.

@yajo The problem is that autobump and optimistic merging are incompatible [1]. That's why I advocate manual bump or as, @max3903 suggests, enabling autobump on a per project basis.

[1] update: if we want to use the version number bump as a release indicator

[sbidoul]

can we package the branches being reviewed and use 'dev' and the PR #?

@max3903 Packaging PR's is possible although not trivial. It's mostly unrelated to the option chosen here. Note it's very easy to pip install from a git branch directly (see my slides), so packaging of unmerged PR's is not necessary IMO.

[yajo]

Well... actually it's not autobump vs optimistic merging. Autobump automates the way OCA currently works, while optimistic merging changes it, so the incompatibility is only in the optimistic merging side of the battle.

That proposal should be discussed in a separate thread, please kindly open it somewhere, expose your arguments, and let's not block this one for that.

[dreispt]

Solutions 1 and 5 are essentially the same thing - manual version bumping, basically as of today, and if possible a Lint check (warning or failing, to be decided).
I don't really understand what Solution 4 is.

[dreispt]

@moylop260 Independently of the autobump discussion, is the "must bump version" check easy to add to pylint?
IMO it is interesting to have it available, regardless of the discussion on how to enable it, or how to automate version numbers.

[yajo]

@dreispt Your questions are already answered; please read the thread 😉

[dreispt]

@yajo, OK I see moylop answered it, and it is "no - it should be done in MQT".

[mmalorni]

Sorry for the long comment!
I vote for #1. I think all automated scripts will be error prone and maintenance heavy as we will not be able to foresee all issues and changes in policies. Since maintainers will do the merge, we can rely on their judgment to decide which digit to increase. This can even help with optimistic merge since in doubt only the last digit could be updated. We can use 6 digits at any point to identify unstable versions if the release process is fixed accordingly. That would help to move things along. Issues can stay open to note what still remains to be fixed and we can merge any PR that has been stalled for more than a week and allow other contributors to make another PR to finalize the work instead of being stuck in the waiting game. Of course, having a dev branch or dev repos would resolve this issue as well and allow everyone to contribute and keep the release branch clean and stable with only PRs that have been vetted by the maintainers. Of course this meens that there might be some PRs with more than one issue fixed or the dev branch becoming filled with unmerged fixes and features (I am being overly optimistic here ;-) .

[moylop260]

Superseded by OCA/oca-github-bot#24