search

OCA / pylint-odoo

Odoo plugin for Pylint
http://www.pylint.org
143 stars 168 forks source link

Add "Must bump version" check #59

Closed dreispt closed 5 years ago

dreispt commented 8 years ago

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 commented 8 years ago

:+1:

moylop260 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

:+1: for adding the check. bumpversion can be a good tool for incrementing the version

dreispt commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

:+1: with the check :-1: for auto-increment

dreispt commented 8 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

+1 with @sbidoul

dreispt commented 7 years ago

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

dreispt commented 7 years ago

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 commented 7 years ago

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 :-1: on this check: encouraging people to bump version at each merge would make it harder to transition to optimistic merge + manual release.

lasley commented 7 years ago

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 commented 7 years ago

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:

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):

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

moylop260 commented 7 years ago

@lmignon What is you opinion for this?

max3903 commented 7 years ago

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

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 https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254953455, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5XsF59vXUMfhY4_a_AIdoSRxrh1aV1ks5q1pKzgaJpZM4JagRd .

moylop260 commented 7 years ago

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

max3903 commented 7 years ago

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: https://github.com/OCA/server-tools/pull/543 2nd PR: https://github.com/naousse/server-tools/pull/1

[image: Ursa Information Systems] _Maxime Chambreuil_Project Manager / Consultant

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

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 https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254956144, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5XsO_KTrIc10X_NCO6pa0IT5Dxrdcfks5q1pWpgaJpZM4JagRd .

sbidoul commented 7 years ago

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 commented 7 years ago

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

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 https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254957592, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5XsOCwvUhoaqMWWdv7ML4ZOditIr62ks5q1pdEgaJpZM4JagRd .

sbidoul commented 7 years ago

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 commented 7 years ago

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):

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 commented 7 years ago

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

lmignon commented 7 years ago

@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 commented 7 years ago

Well, for those who don't like this requirement, let me quote myself from https://github.com/OCA/maintainer-quality-tools/issues/376#issue-183622844:

The rationale behind this is that I'm thinking of some sort of autoupdate addon that updates all addons whose current version does not match the currently installed one. Kinda "update all", but only what is needed and automatically, to get less failures in case an addon gets updated.

As some may have noticed, OCA grows usually more than what most of us can control. And having updatealls all the time is not fun because it is more downtime for no benefit. So the only way to create an addon with a cron that automates updates is to check installed version vs. available version. Well, option B would be to analyze git diffs, but git is not something we actually need in production in these Docker and pip days.

If we want to have such automation, but avoid https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254951868, then I also vote for the proposal of @moylop260 in https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254972623, with an additional benefit of Transifex updates raising versions :tada:. In fact, a real nice check would be one that installs old addon, then runs tests, then updates to new, then runs tests, but that's another issue.

OTOH, I have absolutely no idea on how to do that, I hope someone does...

sbidoul commented 7 years ago

If people want the automation, I suggest a variant with 6 positions: odoo.version.breaking.feature.fix.dev where dev is the number of commits since the last commit changing the version in the manifest.

So we leave breaking.feature.fix as a manual indicator of stable releases (as usual in normal projects), and dev being the automatic increment.

An algorithm to compute dev is here: https://github.com/acsone/setuptools-odoo/blob/master/setuptools_odoo/git_postversion.py

yajo commented 7 years ago

I think that should be odoo.version.breaking.feature.fix-dev then, but in any case I still prefer https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254972623. After all, if a patch landed and no breaking nor feature is incremented, then that means it is a fix (or translation, which can be considered a fix too for people that talk that language).

Besides, adding a commit that tells you how many commits since the last commit is recursive...

dreispt commented 7 years ago

I tend to dislike automatic changes to modules, since it adds more magic and machinery, and that kind of stuff tends to bite us back sooner or later. But I won't oppose.

@sbidoul Nice suggestion. How about the 5 position odoo.version.breaking.feature.dev?

yajo commented 7 years ago

Actually manual things are the ones that break things :stuck_out_tongue_winking_eye:

sbidoul commented 7 years ago

I like to have the breaking.feature.fix part updated manually by the maintainer, to declare stable releases of any type (including bugfix releases).

It's quite common to have an additional position (different from the bug fix position) to mark developmental unreleased versions.

I personally don't need the 6th position to be updated automatically (as setuptools-odoo does it when packaging) but if some people like to have it, I'm perfectly fine with that and I believe the algorithm in https://github.com/acsone/setuptools-odoo/blob/master/setuptools_odoo/git_postversion.py is applicable to do that.

yajo commented 7 years ago

I like to have the breaking.feature.fix part updated manually by the maintainer, to declare stable releases of any type (including bugfix releases).

The problem with that is that we would still hit https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254951868.

@moylop260's solution implies that developers should not manually increase fix, but rely in the automatic trigger instead.

But if the developer never increases fix and the trigger only increases dev, then fix would always be zero, and then fix would become useless, so we could simply remove fix... but then dev becomes fix, so it's the same as removing dev and leaving versioning schema exactly as it is right now.

as a side note: setuptools-odoo updates the pip version, not the version Odoo sees. That must be updated in the __openerp__.py file (__manifest__.py for > 10) for him to see it AFAIK.

sbidoul commented 7 years ago

But if the developer never increases fix and the trigger only increases dev

@Yajo we could extend your reasoning to the other components (breaking, feature). In the end if the maintainer does not do his release management/versioning job, then there is a problem.

My point is that we should not try to reinvent processes in OCA. In the vast majority of projects the 3 main components of the version (major.minor.fix) are bumped manually when a version is stable/released. I see no good reason to do differently in OCA.

yajo commented 7 years ago

Yes, I get your point, but how would you avoid https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254951868 then?

sbidoul commented 7 years ago

@Yajo avoid what exactly? (that comment si very long)

yajo commented 7 years ago

Avoid having several PR open at the same time, all of them bumping the fix number, forcing each of them do a rebase each time another one is merged. After all, no PR has sense without a version increase.

sbidoul commented 7 years ago

Creating PR is the role of contributors. Contributors don't change the breaking.feature.fix.

Updating breaking.feature.fix is the responsibility of maintainers. That must be done after merging if the maintainer deems the PR safe enough to be stable, or later, possibly after merging several PR's.

So IMO, PR's must not change the version.

lmignon commented 7 years ago

Am I wrong by saying that what we need is a proper release policy where the PSC/maintainers are in charge to increase the version number, create a new tag, .... .

yajo commented 7 years ago

I think that is definitely wrong, friends. This thread is about automating the version number bump, either by adding a check or adding an automated action. You are trying instead to move it to a policy change where maintainers have an additional manual burden. Sorry but not, that makes no sense.

Why would a PR get merged if it is not stable enough? Stability must be reached in the PR stage, where the review process takes place, not after merging.

The PR issuer is the one who must comply with all Travis checks, and that's why we were asking for this check in the first place. He is also the one that knows if the change is a breaking, feature or fix. Yes, maintainers can help now that there is the GH tool of allow changes from maintainers, but PR should get cleaned before merging, not after.

And I think you are missing https://github.com/OCA/pylint-odoo/issues/59#issuecomment-254935238.

So IMO, PR's must not change the version.

Guidelines are clear. If you don't increase the version number, is because your patch does not fit into any of those categories. Could you provide an example for that? Personally, I can't imagine any.

lmignon commented 7 years ago

@Yajo IMO OCA lacks of a proper release process. In other communities it's easy to known the list of changes between 2 releases, you are able to see the changes into the code between 2 releases just by comparing 2 tags, .... I don't want to add an additional manual burden on the maintainers. As you can see in one of my previous comments I also supported the idea of an automatic bump of version number. But the comments from others allow me to see the problem from another point of view and finally I wonder if this auto bump is not such a good idea.

sbidoul commented 7 years ago

@Yajo guidelines wrt versioning are clear and correct indeed. I don't question that. Note it's not written in the guidelines that each PR must bump breaking.feature.fix.

What I say is that the current OCA approach where merge = release is considered harmful. This slows down merges and creates stress and risk. Therefore I advocate progressively evolving towards a process that is more standard in open source projects:

Note it is only a small variation on the existing OCA process.

The best/easiest way we have to signal a release is to bump breaking.feature.fix. All I say is that this must be a human decision. If an automated bump is needed it can be done after merge and on a 4th position breaking.feature.fix.dev.

pedrobaeza commented 7 years ago

@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.

lasley commented 7 years ago

IMO the main thing holding us back from a proper release process is having multiple modules in a repo. I don't see a way to perform the tags necessary in any meaningful way, so we seem to need another option.

Sorry if this is a newb question, but how would migrations be affected (assuming we did go with the auto-bump)? Let's say for example:

If the user upgrades from 9.0.1.0.0 to 9.0.2.0.1, is the 9.0.2.0.0 migration still run?

lmignon commented 7 years ago

@lasley

If the user upgrades from 9.0.1.0.0 to 9.0.2.0.1, is the 9.0.2.0.0 migration still run?

Yes it is.

yajo commented 7 years ago

the current OCA approach where merge = release is considered harmful

What you consider harmful I consider one of the best strengths of OCA.

Besides, OCA has stable branches: those with names like 8.0, 9.0, 10.0, etc.; and it has development branches: those with names like pull/###/head, I see no need for change there. You can checkout/merge/open PR to the one you want.

The problem here is that we have some guidelines on versioning, but they are not actually being followed because either devs are not forced to do so or they are not automated, thus there is no reliable way for Odoo to know if an addon had an update.

The solutions AFAICS are:

  1. Add a check in travis. Votes: @pedrobaeza, @dreispt, @mmalorni.
  2. Autobump last number (5th position, fix). Manual bump on others. Votes: @moylop260, @dreispt, @NL66278, @max3903
    • + Most fixes will raise version automatically.
    • + Devs must still do manual update when a breaking change is included.
    • + Can be automated for Transifex commits too.
    • + No changes on guidelines.
  3. Autobump last number (6th position, dev). Manual bump on others. @elicoidal
    • - Changes guidelines.
    • - Aims to merge unstable & stable code in the same branch.
    • + Most fixes will raise version automatically.
    • - Although that would have no benefit because additional manual work should be done by maintainers.
    • + Faster merges (although that's not a + for me...)
    • - Blurs the distinction between a fix and a dev. Seriously, what PR that is not a breaking, improvement nor fix could ever exist anywhere?
  4. Forget this. --update all always.
    • + It works out of the box and right now.
    • - Quite a few downtime in big installations.
    • - Most downtime is spent updating things that are updated already.
  5. Automate nothing, let maintainers change version numbers manually after merges. Votes: @sbidoul.
    • + Contributors forget about version numbers.
    • + Maintainer is responsible for versioning, which is closer to normal open source versioning process.
    • - Nothing is automatic.
    • - If a maintainer forgets to do it, no bot will notice.
    • - Additional manual burden for maintainers.
  6. Let each PSC choose among options 5 and 2, defaulting to 5. Votes: @lmignon.
    • + Nothing changes for a PSC unless he wants to.
    • - Not homogeneous in all OCA.
    • See +/- points of each option.
  7. Trigger autobump from option 2 when [FIX] is found in the PR title or commits come from Transifex; otherwise, trigger linter from option 1. Votes: @simahawk @Yajo @yvaucher @lasley.
    • + Multiple fixes for same addon at the same time would not conflict.
    • + [IMP]s would not forget to bump the version.
    • + It could be extended to autobump other version parts if PR has [IMP], [REF] or [MIG] in the title.

My vote goes for option 7. Cast your votes please.

Please correct me if I forgot some option/pro/con.

As a side note, I'm thinking also on another approach for option 4, that could be also applied for option 2 if ever implemented:

  1. Add a flag to warn your users about maintenance status.
  2. Forbid updates for the currently connected Postgres user, restricting its permissions.
  3. Duplicate the database.
  4. Connect the outdated Odoo to the duplicate.
  5. Restore write permissions in original DB.
  6. Wake up an updated Odoo instance with --update all on the original database.
  7. Shut down the outdated Odoo when the updated one finishes the update.
  8. Wake up the updated one.
  9. Destroy containers for old DB duplicate and old Odoo instance.

Yes, that would take a bigger amount of time, but it would be automatic and would mean close to zero downtime in the user's face. Well, they'd have some read-only time, but that's better than downtime.

Just thinking out loud.

dreispt commented 7 years ago

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.

moylop260 commented 7 years ago

My vote is for option 2

lmignon commented 7 years ago

My vote is for keeping the process manual. If the automatic version bumping is put in place we must provide a way to disable this automatic process.