Skip to Content
  2. Mailing Lists
  3. Contributors
  4. Re: Reviews

Archives

Contributors

Re: Reviews

Re: Reviews

Re: Reviews 

> Also, even if there are 2 reviews then PSC's generally don't respond to

> merge requests.

I can tell you why I don't: It has happened quite often in the past that I 
trusted fellow reviewers' reviews, but afterwards it turned out what I merged 
was very obviously wrong - just as you mention below we do have quite a bit of 
substandard code already in the codebase. And I feel responsible for what I 
merge. So if the existing reviews are not done by (to me) "known good" 
reviewers, I'll only merge if I reviewed myself too, and that is severely 
constrained by lack of time.

In the survey about PSCs earlier (I haven't seen any results of this, will 
those be published?), I wrote that I'd like to have some kind of automated 
triage, this would increase my reviewing/merging output considerably. Very 
often I spend my weekly OCA-time on clicking through PRs that are in my 
opinion of such a low quality that a review would be CS 101 plus guided 
reading of the conventions, and that's simply not what I signed up for.

A bot that closes PRs with failing CI for longer than X days would be a start, 
another bot could close PRs of people who don't play nice (details to be 
determined), inactive PRs should be closed much earlier. Then I know that 
what's left is not a waste of time and go reviewing in a much happier mood.


-- 
Your partner for the hard Odoo problems
https://hunki-enterprises.com
by Holger Brunn - 02:20 - 16 Sep 2021

1 attachments 1 attachments

signature.asc

Reference

  • Reviews 
    Hi all,

For years, OCA has a big problem with unmerged PR's.

Also, even if there are 2 reviews then PSC's generally don't respond to 
merge requests.

The answer has always been "let's review more" or "Let's use 
gitaggregator and so we can use unmerged PR's".

But why don't we try something more radical:

- Let's write a script to assign "maintainer" role for all modules to 
the person who committed the oldest/original version of it

- If maintainer does not respond to a ping longer than 1 month he loses 
the maintainer role, which then changes to the default maintainer that 
is set for the full repo.

- Let's require only 1 positive review from now on. After that the 
maintainer can merge.

--> I don't think that having 2 reviewers is always necessary and it 
also does not prevent bugs from being merged - this happens anyway. I 
used to have high trust of merged OCA modules but after seeing some 
quite ugly bugs and incomplete work being merged I am starting to think 
that maybe the quality of the unmerged stuff is not that bad as compared 
to what is actually merged. The maintainer can prevent really bad 
changes from entering, by just closing the PR.

Tom
    by Tom Blauwendraat - 01:51 - 16 Sep 2021

    10 replies 10 replies

    • Re: Reviews
      Such fw-port tool exists somehow, but in local, thanks to Sébastien Alix:


      But theory is one thing, and the other is the reality: most of the fw-ports will fail due to pre-commit and black, different code due to version changes, refactorings... Automating the creation of the PR simply won't work, as if it's not satisfactory, or requires extra work, you won't be able to push to the fw-port created branch.

      Instead of having a lot of tools, which is not bad, IMO you should instead involve more and more of your people in PSCs and as maintainers inside your organization, or spread the adopt a module campaign for more casual contributors, trying to involve them in this.

      Regards.
      by Pedro M. Baeza - 12:16 - 17 Sep 2021
    • Re: Reviews
      El vie, 17 de sep de 2021 a las 07:41:54 AM, Tom <tom@sunflowerweb.nl> escribió:
      Regarding the "smarter bot" ideas: although it's typical for a bunch of programmers to come up with a solution to a problem by throwing more engineering into it, I feel there's something to it, it just needs more crystallization and then a hackathon to make it happen. 

      Let's take for example Firefox.

      Firefox has the desktop product, and it builds in different platforms: Linux, Windows, Mac... How to handle that? They have a build system that applies conditionals ("if", "case" and similar) to produce the platform-compatible artifact. Usually this is a Makefile, but there are other systems for the same. Result: single code base, multiple platforms.

      However it also has Android and iOS versions, which are almost completely different. They share some essential concepts, but code bases are split. This is more complex to handle, but they end up supporting these extra platforms too. Result: split code base, single platform.

      As you can imagine, the desktop product is more maintainable.

      Now, OCA. Our products are, mainly:
      1. Addons.
      2. OpenUpgrade.
      3. OCA tooling.
      We only follow the "single code base, multiple platforms" approach in OCA tooling.

      For OpenUpgrade, we use "split code base, single platform", and it would be impossible to change that.

      And for addons? The platforms they run on are different versions of Odoo. I always believed it's possible to have a single code base, but that's a dead discussion and simply won't happen, so the best we can do is improve what we have. And what we have is "split code bases, single platform". And IMHO the way to improve it is a fwbot, just like Odoo does.

      Maybe a good theme for a team at this year's online code sprint?

      Indeed.
      by Jairo Llopis - 12:01 - 17 Sep 2021
    • Re: Reviews

      Regarding the "smarter bot" ideas: although it's typical for a bunch of programmers to come up with a solution to a problem by throwing more engineering into it, I feel there's something to it, it just needs more crystallization and then a hackathon to make it happen. Maybe a good theme for a team at this year's online code sprint?


      As a reminder, OCA Days are coming up, and there will be the usual code sprint.
      It would be nice to have a group working on OCABot improvements.
      This can be to work both on code or process change proposals.


      A question that popped to my mind yesterday is one for the board - has it ever been considered for OCA to pay people to fulfill maintainer roles within the organisation? Money seems to be something thats available, and the spending of money on OpenUpgrade has been a great things throughout the last years also.

      It has been discussed.
      The feeling was that doing so it would work against the volunteer spirit of the community, and could break volunteers motivation.
      Investing on better tooling would be better.
      For the record, there has been an initiative for bounties, where people could publish a bounty, awarding a payment for a task, but it didn't get traction.


      Thanks
      Daniel



      by Daniel Reis - 10:25 - 17 Sep 2021
    • Re: Reviews
      Wow, i must say that everyone comes up with much better ideas than I had originally.

      Regarding the "smarter bot" ideas: although it's typical for a bunch of programmers to come up with a solution to a problem by throwing more engineering into it, I feel there's something to it, it just needs more crystallization and then a hackathon to make it happen. Maybe a good theme for a team at this year's online code sprint?

      I hope more people keep contributing to this thread since I do think it's a big issue. I'm also interested to know how other open source projects are handling this. And perhaps we can look at the "IT projects LLC" repos for inspiration - even though i dont understand their system, it looks pretty happy with all the icons. A "important bugfix" being represented by an ambulance.

      A question that popped to my mind yesterday is one for the board - has it ever been considered for OCA to pay people to fulfill maintainer roles within the organisation? Money seems to be something thats available, and the spending of money on OpenUpgrade has been a great things throughout the last years also.

      Thanks all

      Sep 17, 2021 09:02:07 Yann Papouin <ypa@decgroupe.com>:

      +1 for the bw/fwport bot

      - It would be nice to have a new convention to tag a PR with a label like "Review it in 30 seconds" [R30] when it is a basic/simple/quick fix add/fwd that does not require you to soak up the functioning.
      - It would be nice too that a bot automatically tag an issue/PR with a github label based on the issue/PR name or simply the target branch name : eg: [12.0] -> image.png


      --
      Yann PAPOUIN, Ingénieur R&D | DEC


      Le ven. 17 sept. 2021 à 08:52, Jairo Llopis <jairo.llopis@tecnativa.com> a écrit :
      I should add that repos using the template (all AFAIK) can upgrade to the latest version and benefit from https://github.com/OCA/oca-addons-repo-template/pull/63. A bot will warn and close stale PRs/issues.


      El jue, 16 de sep de 2021 a las 12:51:48 PM, Sylvain LE GAL <sylvain.legal@grap.coop> escribió:
      That's true ! One of the origins is the fast release rate of Odoo, which requires continuous migration of modules that, for many, do not change.

      Unfortunately, that situation will not change. Odoo policy seems to be : "a year = a release".

      Yep. They do it because they can handle it. The question is: why we can't?

      There are many reasons, but the one that I find most important and easiest to fix is that we need a bw/fwport bot.

      That will make maintenance pleasing, so we'll have more maintainers.

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      by Tom Blauwendraat - 09:40 - 17 Sep 2021
    • Re: Reviews
      +1 for the bw/fwport bot

      - It would be nice to have a new convention to tag a PR with a label like "Review it in 30 seconds" [R30] when it is a basic/simple/quick fix add/fwd that does not require you to soak up the functioning.
      - It would be nice too that a bot automatically tag an issue/PR with a github label based on the issue/PR name or simply the target branch name : eg: [12.0] -> image.png


      --
      Yann PAPOUIN, Ingénieur R&D | DEC


      Le ven. 17 sept. 2021 à 08:52, Jairo Llopis <jairo.llopis@tecnativa.com> a écrit :
      I should add that repos using the template (all AFAIK) can upgrade to the latest version and benefit from https://github.com/OCA/oca-addons-repo-template/pull/63. A bot will warn and close stale PRs/issues.


      El jue, 16 de sep de 2021 a las 12:51:48 PM, Sylvain LE GAL <sylvain.legal@grap.coop> escribió:
      That's true ! One of the origins is the fast release rate of Odoo, which requires continuous migration of modules that, for many, do not change.

      Unfortunately, that situation will not change. Odoo policy seems to be : "a year = a release".

      Yep. They do it because they can handle it. The question is: why we can't?

      There are many reasons, but the one that I find most important and easiest to fix is that we need a bw/fwport bot.

      That will make maintenance pleasing, so we'll have more maintainers.

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      by Yann Papouin - 09:01 - 17 Sep 2021

      1 attachments 1 attachments

      image.png