[14.0] ADD upgrade_analysis

[legalsylvain]

Work in progress. Do not review for the time being.

Done

• rename module openupgrade_records into upgrade_analysis.
• recover module openupgrade_records and odoo_patch, preserving author.
• full refactor using new odoo convention, pre-commit, ...
• in the upgrade.comparison.config get the version of the target odoo.
• replace the transient model openupgrade.analysis.wizard by a non transient model upgrade.analysis. it allows to make many analysis. it allows also to add in a second time, more metrics, (like number of changes, number of new models, ...)
• in the upgrade.install.wizard, add the possibility to choose the modules to install (all the Odoo module, all the OCA modules, custom modules, etc...)
• files are now written in the path of module get_module_path, if it's a "non Odoo SA module" (OCA modules, custom modules) or in the upgrade_path setting for the official modules. (this setting has to be present in the odoo.cfg file or with the option --upgrade-path). (introduced here : [IMP] ORM: Add new --upgrade-path CLI option odoo/odoo#32650)
• remove useless patches that are not needed for this module. Ref : [RFC] Full refactor of the design of the project OpenUpgrade OpenUpgrade#2440 (comment). Done here [RFR] Improve patchwork legalsylvain/server-tools#3 by @StefanRijnhart.
• Check if all patches are required in addons and for the function quirk_standard_calendar_attendances. (done here [RFR] Improve patchwork legalsylvain/server-tools#3 (comment)))

Todo :

• rewrite readme files
• update the helper script available here to make it working with any version
• handle the apriori duplication. (or not).

Usage :

• install the module upgrade_analysis
• (optional) install modules via the wizard
• generate records
• Create a comparison configuration and test the connexion
• Click on "New Analysis"
• Click on "Perform Analysis"

[StefanRijnhart]

My PR here legalsylvain/openupgrade-framework#1 improves on some of the patches and it adds a mechanism to apply a collection of patches only when we need them. Would you consider taking some of these changes? I can propose a PR onto your branch here.

[legalsylvain]

Hi @StefanRijnhart. Yes, I like your OdooPatch class. I think it's a great improvment.
If you have time, yes feel free to contribute on that branch and make a PR. (If you prefer, I can give you write access on my repo, and we will squash commits at the end of the refactor.)
Let me know. I'll not work again on that topic this week.

[StefanRijnhart]

You can find my PR into this branch here: legalsylvain#3

[legalsylvain]

Hi @StefanRijnhart :

1. it seems travis is broken because of this line : https://travis-ci.com/github/OCA/server-tools/jobs/451380734#L796
   did you missed to commit the removal of an import here https://github.com/OCA/server-tools/pull/1941/files#diff-3008ab525e7f6378f5fc9b15bda124e1a301da8b39ba4f4589b92671c2192e65R2 ?

2. when I made the split of the openupgrade project into two project (upgrade_analysis and openupgrade_framework I saw that a file was required for both part the apriori.py file sample here for V13)

As far as I can understand there dict are used :

• for the "analysis" part, to understand which module has been renamed / merged. in the compare.py function here.
• for the "upgrade" part, to make some operations at the beginning of the upgrade process. (here, for V13)

We have basically three options :
A) duplicate the file and the data.
B) make an optional dependency from a module to another.
As the openupgrade project allways requires this files to work properly and upgrade_analysis requires this file only to analyse odoo SA modules differencies, i propose to update the file apriori.py in the module upgrade_analysis with somethink like that :

try : 
    from odoo.addons.openupgrade_framework import apriori
except:
    apriori = False

[...] 

if apriori:
    renamed_modules.update(apriori.renamed_modules)
    merged_modules.update(apriori.merged_modules)

It adds the constraints to have the openupgrade_framework module in the addons path when running analysis.

C) make a standalone module openupgrade_module_changes with this file and make both module upgrade_analysis and openupgrade_framework depends on this simple module. Maybe more visible. also, when porting the module openupgrade_framework we never have to port the data from a version to another, so it makes sense to have a distinct module.

My point of view :

I prefer to avoid A, because it is duplicating the data.

I maybe prefer C, but B is also OK. I just think that most of the changes of this file are done when "upgrading", so anyway, it makes sense to set the data in the Openupgrade repository better than in the server-tools repository.

What do you think ?

Note : it doesn't seems to be a huge trouble for the current migration (13 -> 14) because there does not seem to be any renaming or module merging between these two versions. Ref : OCA/OpenUpgrade#2190

[StefanRijnhart]

Thanks for noting the travis breakage. Will look at that tonight. I think apriory belongs to openupgrade_scripts rather than the framework, because it is different for every edition. Maybe we should try to import apriori from upgrade_path? I can implement that if you like.

[legalsylvain]

think apriory belongs to openupgrade_scripts rather than the framework, because it is different for every edition

yes you're right ! it seems the best solution. 👍

Maybe we should try to import apriori from upgrade_path ?

Not sure to understand your proposal.

• do you mean config.get('upgrade_path') ? If yes, it is now optional, due to https://github.com/legalsylvain/server-tools/pull/3/files#diff-e768c0bad8381e3d5a9cef3f3193890e5438592d5e9da9486c523bb55b19805aR37 so we can no longer rely on this value.

• if you mean upgrade_path (of modelupgrade.analysis) it is too late, because apriori is required in the first step (generating records) so, analysis is not created and it will not allways work with the "automatic script". (https://github.com/OCA/OpenUpgrade/wiki/Crude-script-to-create-the-full-analysis-between-two-versions-of-Odoo)

For the time being, I see the following solution as the more simple

try:
    from odoo.addons.openupgrade_scripts import apriori

We can, in that case, add a warning in the logs like openupgrade_scripts addons is not available on your system. Your analysis may not be complete if you try to analyse changes of Odoo SA or OCA modules

[StefanRijnhart]

Not sure if openupgrade_scripts will show up under odoo.addons as it is not necessarily a python module (or an Odoo addon), but if it works that would be best. Otherwise we can add the upgrade_path (or apriori path) to other wizards as is needed, using config.get('upgrade_path') as a default again.

[legalsylvain]

Not sure if openupgrade_scripts will show up under odoo.addons as it is not necessarily a python module (or an Odoo addon), but if it works that would be best

Indeed, openupgrade_scripts should be an odoo modules if we want the import of odoo.addons.openupgrade_scripts to work.
For the time being, I designed the refactor in this way : See : https://github.com/legalsylvain/openupgrade-framework/tree/14.0-upgrade-pocalypse/openupgrade_scripts

Is it an odoo module ? not really I confess, and there is no need to install it, and install it will do nothing. (as openupgrade_framework BTW that only requires to be present in server_wide_modules). The original reason is the visibility of such design. If it is a module, it can be available for download on OCA appstore, and maybe on odoo appstore (if openupgrade word is not blacklisted ;-) ... ), also available on pypi without any actions. So easy to download, easy to "deploy". also it comes with a nice readme generated with fragment if we want to add extra explanation, or roadmap section). We can also use merge patch | minor command of ocabot to merge PR.

Then, the instruction for end user will be quite simple :
For an upgrade,

1. get and install odoo in the way you prefer.
2. download both openupgrade_framework and openupgrade_scripts modules in the way you prefer. (pypi, git, download via stores)
3. add a key in your odoo.cfg file. (the upgrade_path)
4. update all.
5. Pray. (Unfortunately, this step will be difficult to remove ;-))

I've already talked about the importance of simplifying the use of openupgrade here. OCA/OpenUpgrade#2440 (comment)

do you see any negative effects to encapsulate the migration scripts in an odoo module ?

[StefanRijnhart]

OK, disguising the scripts directory as a module is fine. I'll use its location on the file system as a default for upgrade_path instead of the configuration key then.

[pedrobaeza]

I'm not sure of understanding about openupgrade_scripts, but if your intention is to encapsulate all migration scripts there, that's a very bad option. We should use upgrade-paths or similar argument line option, and respect the addon hierarchy in a separate repo with that scripts. We can even add migration scripts from other OCA addons.

[legalsylvain]

I'm not sure of understanding about openupgrade_scripts, but if your intention is to encapsulate all migration scripts there, that's a very bad option. We should use upgrade-paths or similar condos line option, and respect the addon hierarchy in a separate repo with that scripts. We can even add migration scripts from other OCA addons.

openupgrade_scripts is the place to store all the migration scripts for Odoo SA addons only. https://github.com/legalsylvain/openupgrade-framework/tree/14.0-upgrade-pocalypse/openupgrade_scripts/scripts

if it's not an odoo SA modules, (custom, OCA) the analysis file will be located as usual, in the migrations folder of the module and it will work correctly.

[pedrobaeza]

Then I insist that shouldn't be the way to go. Scripts should be outside any Odoo module, and follow the same structure as previously:

account/migrations/14.0.1.0/*

and then use them with --migration-paths (or similar name) option when launching the upgrade. This is also the way Odoo does it.

This allows to have a more intuitive structure, we can add more Odoo major version scripts from other OCA repos, etc.

Controlling a mega-module with all the Odoo core scripts has no sense for me, it requires a big maintenance work, and even the version bumping you are commenting doesn't serve for anything. What a minor/major/patch version bumping means?

[StefanRijnhart]

@pedrobaeza thank you for that pointer. I see Odoo supports a --upgrade-path parameter for this. I agree we should use this for the core modules instead of a pseudo Odoo module.

As for your suggestion to also store scripts here for OCA modules etc., I am not sure. I think keeping them within the actual module has its advantages.

[StefanRijnhart]

And apologies @legalsylvain for not recognizing what the upgrade-path configuration key, which you already used in the code, stood for.

[pedrobaeza]

I'm talking about only major Odoo versions of OCA modules. Advantages of this approach:

• Everything in the same place.
• We can avoid large CIs execution for something that is not being tested.
• We can maybe add CI to these scripts using same approach than with OpenUpgrade.
• Regular users of the OCA module doesn't need these migration scripts. Only DB migrations.
• On auto-update systems based on checksums, don't trigger an update for those added migration scripts.
• Catch attention for reviewers having them in the same repo, and not split through all the OCA repos.

For major changes on the module that require migration scripts, of course they still should be included inside OCA module itself. Example: OCA/credit-control#86

[StefanRijnhart]

For me, commiting the migration script together with the migration of the module code is 'having everything in the same place', as well as having the reviewers review both together.

The other points you mention about the infrastructure overhead of the additional changes, I don't think they are a big problem because migration scripts for modules, even between major versions, are rare, especially if the migration script is committed together with the module code which must surely be the most common case.

[pedrobaeza]

@StefanRijnhart migration scripts for a change of version are not committed at the same time as the migration module. The more recent example is in all the account related stuff: only when we have cleared the structure of the account migration scripts and which fields we rename/fill for linking old objects with new, we have been able to create migration scripts for all the OCA modules that touch invoices.

But meanwhile the module can be migrated for being used in the new version for those not migrating DB. And not only that, you can't demand a contributor to provide migration scripts for migrating the module from an old version when contributing the module migrated to the new one.

About the CI overhead, I can assure you is not pleasant to wait 15/20 minutes for nothing. I commit directly sometimes for avoiding that thing and due to there's no real gain on running such CI.

[StefanRijnhart]

Everybody knows what it's like to wait on the CI. But that's not the same thing as the load that migration script PRs add to the total CI of the OCA. I'm sure that the merge of the invoice models in the move models caused a flurry of migration scripts when the first database migrations were being developed, but in general I think it's not unreasonable to expect the migration script in the code migration PR. I have seen that many times to be the case.

[pedrobaeza]

OK, we can leave such part to the contributor decision of doing one way or another, as anyway they will need to provide the general upgrade path for doing the migration, and also the addons path for the OCA modules.

[legalsylvain]

Hi pedro & Stefan,

1) regarding pseudo module

I think that there is some misunderstood.

(@pedrobaeza said : ) Then I insist that shouldn't be the way to go. Scripts should be outside any Odoo module, and follow the same structure as previously: account/migrations/14.0.1.0/*

My bad ! Indeed, for the time being, analysis is generated in openupgrade_scripts/scripts/account/14.0.1.1/openupgrade_analysis.txt
and the /migration/ part is missing. I think that it is fixed now and that the new analysis file should be generated with the good path, I mean : openupgrade_scripts/scripts/account/migrations/14.0.1.1/openupgrade_analysis.txt

(@pedrobaeza said : ) and then use them with --migration-paths (or similar name) option when launching the upgrade. This is also the way Odoo does it.

Yes, that is exactly what I propose ! running odoo with that option.
--upgrade-path=[...]/openupgrade_scripts/scripts/ should work with the current PoC.

(@StefanRijnhart said : ) @pedrobaeza thank you for that pointer. I see Odoo supports a --upgrade-path parameter for this. I agree we should use this for the core modules instead of a pseudo Odoo module.

That is not incompatible. I propose to use a pseudo odoo module only to make the script more accessible and visible. But my objective is to stick to the standard and use the --upgrade-path option.

(@pedrobaeza said : ) Controlling a mega-module with all the Odoo core scripts has no sense for me, it requires a big maintenance work

I don't get it. What I propose is simply to put the main script folder of the OpenUpgrade project (that contains all the analysis and the migration files) in a folder that contains a __manifest__.py file. Nothing more. Could you explain "the big maintenance" generated by this move ?

2) Where are the migration scripts of the OCA modules ?

(@pedrobaeza said: ) migration scripts for a change of version are not committed at the same time as the migration module

Well, it depends. Two situations :

• "Port without refactoring" : a module exists in a previous version (like 12.0). When a new version is available (14.0) contributors will port the module to have it available in recent version and they don't care about the migration from an old version because they just want to use it for a new customer, for the recent version.
  In a second time, people that are migrating huge instance, with a lot of OCA modules will want to have the migration scripts and so develop them. For that extra works, they often requires extra features that can be in the openupgradelib project. (like function to handle partner refactor between 6.1 and 7, or to handle binary field change from db to attachement (version 9+))
  -> made in two steps, with two distinct contributors : you're right.

• "Port with refactoring" : in some cases, maintainers will take advantage of a new version to make deep refactoring. In that case, they will develop in the same time, migration script AND the port / refactor of the module. A recent personal exemple : 12.0 mig web dashboard tile web#1428
  Another example (futur) : I want to refactor the mass_editing module to inherit of core server.actions model and remove useless code.
  -> made in one step, with a single contributors : @StefanRijnhart is right, it is annoying in that case to have to make 2 PR, in two repositories, making one PR waiting for the second one.

We can avoid large CIs execution for something that is not being tested.
Regular users of the OCA module doesn't need these migration scripts. Only DB migrations.
On auto-update systems based on checksums, don't trigger an update for those added migration scripts.

Well, there are not a lot of migrations folders in the 2000+ OCA modules. I think it doesn't concern more than 5% of the modules in each version. Interesting point, but not a blocking one for me.

We can maybe add CI to these scripts using same approach than with OpenUpgrade.

That could be great ! But it requires extra work to make a specific CI.

But but there's another point you didn't address : mixted migration of instance on premise with EE modules (and contract) and OCA modules. (I may be wrong because I never used that service, so excuse me if it's not relevant)
If a user want to upgrade a instance with EE modules and OCA modules installed, I guess :

• he will dump the database on https://upgrade.odoo.com/database/upload.
• Odoo will make the upgrade of the odoo modules, letting OCA modules in a to upgrade state and provide the migrated database.
• Then user has to finish the migration, making a full --update all with all the ported modules (OCA / custom) of the targeted version. In that step, the migrations scripts will be executed.

But until now, user doesn't have to use openupgrade project for that purpose. if the final update is done without the openupgrade project, the migration will fail silently. We can assume that EE users doens't know very well the openupgrade project.
But again, I don't know very well all the EE services, and I'm not aware of the recent new services with extra supports, which provides support (and maybe migration) of third party modules at a very competitive price. For example :

[StefanRijnhart]

I've submitted a PR on this PR's branch to fix the import error and refactor apriori handling. It is now imported from the --upgrade-path. It seemed safer to do this as JSON rather than a Python import.

legalsylvain#4

Meanwhile, the resulting analysis and an empty apriori.json are added here: legalsylvain/openupgrade-framework#2 (oh, and I'm very sorry but I removed the manifest from openupgrade_scripts because I can see another poll coming, or more than one).

[legalsylvain]

I've submitted a PR on this PR's branch to fix the import error

thanks !

It seemed safer to do this as JSON rather than a Python import.

I have to think about it.

(oh, and I'm very sorry but I removed the manifest from openupgrade_scripts because I can see another poll coming, or more than one)

Well, I would like to wait for valid counter-arguments from @pedrobaeza (or other people) that would indicate why a module is a bad idea. For the time being, I only see improvment explained here. #1941 (comment)

[pedrobaeza]

I still don't see any advantage of using such complex scheme. You are saying it's better to have openupgrade_scripts/scripts/account/migrations/14.0.1.1/openupgrade_analysis.txt than account/migrations/14.0.1.1/openupgrade_analysis.txt inside OCA/OpenUpgrade?

About your arguments for being better:

• Why do you want to do from odoo.addons import openupgrade_scripts? It's not even possible to import anything inside 14.0.1.1 due to the dot notation, so it doesn't serve for anything.
• I repeat my same question about for what we want to bump versions. What a minor/major/patch version bumping means in this context? You should know that we can trigger the same the bot merge command without having modules. It will work the same. We are already doing it on OpenUpgrade itself.
• You have to put as --upgrade-paths <download_folder>/openupgrade_scripts/scripts instead of only <download_folder>. That's not favoring at all the simplicity.
• You are talking about publishing to pip, but that doesn't serve without the infamous setup directory and other required steps that will complicate the CI of the project (that's why I'm telling about maintenance). It's far easier for newbies IMO to simply download the ZIP from GitHub UI, uncompress it, and point with --upgrade-paths to that uncompressed folder.

About my proposal for OCA modules, you have to do 2 PRs indeed, but you don't have to wait if you don't want, unless we enable a CI for such modules, but if we enable it, people will want to test such migration scripts and will prefer to do both PRs for that reason for sure.

For EE migrations, there's also no theoretical problem: people still download whole OpenUpgrade repo and apply the same --upgrade-paths for the second round with --update all, and Odoo core modules won't be touched, as they already have the latest version updated in ir_module_module table and no migration scripts from OpenUpgrade will be triggered. Only OCA modules ones.

Anyway, don't be naive: nearly 0 OCA modules migration scripts will work with EE migrations. Why? Because these migration scripts expect certain data scheme that belongs to OpenUpgrade, not the way Odoo EE handles the migration. Some examples:

• Use of openupgrade_legacy_xx_* columns.
• In v13, we populate a column old_invoice_id for account.move and such column is used for filling fields that were in invoice and now must be in moves. I doubt Odoo uses the same column name.

Anyhow, I'm telling to do it as an option. Contributors that still want to bundle everything together, can do it as well.

[StefanRijnhart]

Sylvain mentions the visibility of the migration project, having the module with the scripts on the apps store for instance. I find that slightly misleading. But the visibility of the project could be a reason to keep the migration scripts in the module directories in the case of OCA modules, so as not to completely isolate the regular contributor from these scripts (and indeed, encourage them to contribute to those as well).

[yajo]

🤔 I think what we need to answer before is: when you add --upgrade-path, migration scripts from the module's migrations folder are still loaded and processed? Because, if yes, then the whole argument is pointless, as we will be able to put the script inside OpenUpgrade or inside the module, depending on each specific situation (module needs, developer preferences...).

[pedrobaeza]

AFAIK all are processed, no matter its source

[legalsylvain]

@yajo, @bealdav could you update your review. I think we can merge this PR, as all is green, and the generation of files are OK. (https://github.com/OCA/OpenUpgrade/tree/14.0/openupgrade_scripts/scripts)

regards.

[yajo]

Don't you need to do a git install from master for openupgradelib? AFAIK the pypi version is always bad...

[bealdav]

I confirm it's a bear trap to install openupgrade lib from pypi

[legalsylvain]

Well. Not sure to understand. When executing a migration with openupgrade project, yes we have to have the up to date library, because some function are oftently added / refactored / etc...

But for that module (upgrade_analysis) it is using only a function table_exists here and that function is quite simple & stable.
https://github.com/OCA/openupgradelib/blob/master/openupgradelib/openupgrade_tools.py#L30

But, if you prefer, I can replace openupgradelib by git+https://github.com/OCA/openupgradelib.git@master

[pedrobaeza]

It's better to always put the latest one, as we can even patch that method or improve this module using other methods of openupgradelib when needed.

[legalsylvain]

done.

[legalsylvain]

Hi. I have a problem with that change. It is making fail the pre-commit process :

Generate requirements.txt for an addons directory........................Failed
- hook id: setuptools-odoo-get-requirements
- files were modified by this hook

Could you say me what to do ?

thanks !

[yajo]

ah... 🤦🏼‍♂️

Could you please try just removing the openupgradelib line from here? I guess that dependency should be present in CIs always... (but I'm not sure)

[legalsylvain]

Well, I can not remove this line, because pre-commit readd this line ;-)

Generate requirements.txt for an addons directory

I so commited again what pre-commit suggests. (I mean openupgradelib in requirements.txt)

I propose to merge this PR as it. if someone see a better implementation, feel free to improve in another PR.

[pedrobaeza]

/ocabot merge nobump

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-1941-by-pedrobaeza-bump-nobump, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at caee2df. Thanks a lot for contributing to OCA. ❤️