[RFC] Red CI and how to go ahead

legalsylvain commented 3 years ago

Hi all,

Travis Status

Travis is red on 8.0, 9.0, 10.0, 12.0, 13.0, 14.0 branches. (since monthes / years)
Travis is green on 11.0 (maybe because there are only 5 modules ;-))

Runbot Status

runbot is red on 12.0 and 14.0
runbot is green on 10.0 and 13.0
runbot is anavailable for 11.0

That is quite problematic, mainly because all the contribution PRs are so red, even if there are valid. Included trivial ones. (see)

The work to make all the branch green is big, and trying to fix a problems raise other problems. see : https://github.com/OCA/l10n-france/pull/286 by @ivantodorovich. flake8 / pylint / pre-commit issues ; no references to oca repositories ; trouble in python lib ; other errors ; ...

To go ahead, I so quick-fixed

12.0 (because that the branch I'm working on). excluding 6 modules.
14.0 (because that is the "head" branch"). excluding 10 modules.
on 12.0 and 14.0, I disabled many modules in the .travis.yml file. (EXCLUDE syntax)
on 14.0, I also excluded many modules in the .pre-commit-config.yaml file.
For all that modules, I set the development_status key to Alpha

So CI is now green, and it should stay green !

However, it is not respecting OCA guidelines. (even for alpha modules, see oca conventions)

I don't know how to continue in the good way.

we keep as it, excluding most of the modules.
we fix the branch. (at least the 14.0 one). @alexis-via : if you want help to understand new OCA quality tools and checks, please ping me, we can work on that topic.

In the meantime :

please stop merge without ocabot command, as it will make the repo red again.
you will soon be able to use the ocabot rebase for your PR against 12.0 and 14.0 branches.

CC : recent contributors : @alexis-via, @kevinkhao, @bealdav, @ivantodorovich

CC : OCA quality-tools maintainers : @sbidoul, @pedrobaeza

pedrobaeza commented 3 years ago

Skipping CI shouldn't be an option. I think at settings level, you can prevent to merge directly things like in odoo/odoo.

ivantodorovich commented 3 years ago

Skipping CI shouldn't be an option. I think at settings level, you can prevent to merge directly things like in odoo/odoo.

That would be great. I couldn't find any feature to prevent merging PRs, but maybe the same could be achieved with this:

ivantodorovich commented 3 years ago

btw thanks a lot @legalsylvain for doing this. It's good to see them green :clinking_glasses:

legalsylvain commented 3 years ago

Skipping CI shouldn't be an option

I'm fully agree with that and I don't do the same thing at all with the OCA/pos repo I maintain.

But in that case, better a partial CI that no CI. ;-)

I think at settings level, you can prevent to merge directly things like in odoo/odoo.

I think blocking that possibility could block some maintenance operation. (for exemple, if oca quality maintainers decide to add some rule / change in default configuration, they want to push on all the head branches anyway. Don't you think ? Also I fear that "waiting for all green" test can block PR, if coverage decreased. (that is allowed for the time being). (coverage can not only increase).

Better to wait the point of view of @alexis-via who is the main commiter of this repository.

btw thanks a lot @legalsylvain for doing this. It's good to see them green clinking_glasses

thanks ! Quite boring job, but I'm glad I did this. l10n-france is the only repo that is always red and that I use in production. It's not a comfortable situation.

alexis-via commented 3 years ago

Happy to see some interest in having Travis/pre-commit green on OCA/l10n-france. When I opened a ticket about red tests in 2018 on v10.0 (https://github.com/OCA/l10n-france/issues/155), nobody cared... happy to see that it's not the case any more! AFAIK, on v14.0, there are mainly 2 problems: 1) some "original" files needs to be excluded from pre-commit. For exemple, l10n_fr_intrastat_service/data/des.xsd -> this file is the original file from the administration, so I don't want to modify it with isort, because we would not be able to see easily if the administration has changed the file or not. At that time, I tried to find some info about how to exclude a file from pre-commit check, but I didn't find the answer. 2) l10n_fr_fec_oca raises many pre-commit SQL warnings. I don't know how to solve them exactly. David Beal tried to fix the warnings in the past but he broke the functional behavior of the module and I had to revert his changes. I don't know if it's a good idea to fix the warnings themselves or if we should just disable the warnings on the file l10n_fr_fec_oca/wizard/account_fr_fec_oca.py

The other issues are small details that are very easy to solve.

I must say that PR https://github.com/OCA/l10n-france/pull/298 is completely ridiculous. It doesn't try to solve the errors, it just disables most of the modules. @legalsylvain You say you have experience with Travis/pre-commit setup (I don't have this experience myself), so it would have been very easy for you to solve most of the errors. OK to exclude l10n_fr_fec_oca because this one has a lot of warnings which are difficult to solve, but the others are pretty easy to fix for someone with experience on travis/pre-commit. I was expecting a much better contribution from you on this topic, I'm very disappointed. I just hope that it was a temporary PR waiting for a second PR that would solve the problems themselves (at least the easy ones).

rvalyi commented 3 years ago

Happy to see some interest in having Travis/pre-commit green on OCA/l10n-france. When I opened a ticket about red tests in 2018 on v10.0 (#155), nobody cared... happy to see that it's not the case any more! AFAIK, on v14.0, there are mainly 2 problems:

some "original" files needs to be excluded from pre-commit. For exemple, l10n_fr_intrastat_service/data/des.xsd -> this file is the original file from the administration, so I don't want to modify it with isort, because we would not be able to see easily if the administration has changed the file or not. At that time, I tried to find some info about how to exclude a file from pre-commit check, but I didn't find the answer.

That one is easy and I told you about it a few days ago, it's similar to https://github.com/OCA/l10n-france/blob/14.0/.pre-commit-config.yaml#L10 except you would specify the exact file or sub-directory wou want to exclude from the rewrite (which is legit). We do it here for instance in OCA/l10n-brazil https://github.com/OCA/l10n-brazil/blob/12.0/.pre-commit-config.yaml#L6 or here https://github.com/OCA/l10n-brazil/blob/12.0/.pre-commit-config.yaml#L8

Also, I tend to think partial CI is better than no CI, so it had to start at some point and I prefer to see the half full glass. As for merging in 24h or setting your modules as Alpha yes I agree this was borderline.

Good luck with the other fixes.

sbidoul commented 3 years ago

Folks please keep cool. I know CI is sometimes annoying... but in repos shared between multiple maintainers as we have in OCA, keeping it green is a matter of courtesy for other maintainers. And then ocabot merge command will run all the checks for you. So please forget the merge button. And if you are in a hurry, leave the PR open and install at your customer with gitaggregate or pip install "odoo14-addon_name @ git+https://github.com/OCA/repo@refs/pull/PR/head#subdirectory=setup/addon_name".

A few general notes from my side:

If you have situations where CI gets in the way, feel free to raise on issue on the template repo. I (and other) try to monitor that one and help as much as we can.
In general, try not to modify the template-generated files (such as .pre-commit-config.yaml or .travis.yaml), as it may create merge conflict when we update the template. If you must, also raise an issue on the template repo to see if an option can be added to the template, or if an OCA-wide solution can be found.

Regarding the specific issues in this repo, I did not look at the test failures if there are any. I did run pre-commit to see where things stand. It's not so bad. Here are the issues and suggested (easy) solutions:

SQL injection risk. Use parameters if you can. - More info https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#no-sql-injection: there are solutions when you construct queries dynamically (see psycopg AsIs). But if you know what you are doing, adding pylint: disable=sql-injection on the offending line will allow you to duck the problem.
C901 'AccountFrFecOca.generate_fec' is too complex: split the long method
B007 Loop control variable 'cell_content' not used within the loop body. If this is intended, start the name with an underscore.: the solution is in the message
It does not complain about xsd files. Perhaps they have been auto-formated already ? If needed, they can be excluded from prettier using .prettierignore. I personally think .xsd files should be ignored globally in OCA. Feel free to raise an issue in the template repo to discuss this.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

legalsylvain commented 2 years ago

I'm sorry to reopen this issue. The CI was all green some monthes ago. But for now :

V15 : green
V14 : green
V13 : Red. (travis)
V12 : Red (Runboat)
V11 : Red (runbot) maybe not a big deal, because Runboat is green and travis also.
V10 : Red (Travis + runboat)

So I see again PRs merged directly without using ocabot command.

How to go ahead ?

CC : @OCA/local-france-maintainers

alexis-via commented 2 years ago

You cannot say "CI was all green some monthes ago" ; it is not true.

I made the work to have Travis green on v14 (PR https://github.com/OCA/l10n-france/pull/302) and on v12 (PR https://github.com/OCA/l10n-france/pull/303). AFAIK, nobody made the work to make Travis green on other branches. For example, as explained in one of my comment, v10 is red because of a failed check in guewen's module hr_holidays_jour_ouvrable (cf https://github.com/OCA/l10n-france/issues/155) and nobody solved it so far.

v14 and v15 are green. On v12, travis is green (only runbot is red). I believe those branches are the most important...

legalsylvain commented 2 years ago

"CI was all green some monthes ago"

you're right ! I don't remember that point, but the work regarding greenification was only about the main branches.

On v12, travis is green (only runbot is red).

yes, for me that is an important branch, and it was green.

What your proposal ? conserve green branch on 14 & 15 and let other branches red ?

thanks.

alexis-via commented 2 years ago

I am willing to do the work for v10.0. Who is willing to do it for v11 and v13 ?

legalsylvain commented 2 years ago

I dont use 11 and 13 and my oca investment is more about openupgrade and ocabot.

But i can try to fix v12 branch I still use.

alexis-via commented 2 years ago

arf, ok. If nobody is motivated, then we'll keep these old branches red and just focus on keeping v14+ branches green.

OCA / l10n-france

[RFC] Red CI and how to go ahead #299