[RFC] Full refactor of the design of the project OpenUpgrade #2440
Comments
|
Thanks, I like it! |
|
thanks for your answer ! I have a design question : there are two kind of patches in OpenUpgrade.
For the time being, all the patch are in the same module I think that it could reduce the quantity of patches in the upgrade process, and so limit the possibility or errors. for exemple, I don't think that such patch in V13 and so in V14 has nothing to do in an upgrade process IMHO. My question : do you think it's relevant to split into 2 folders of patches ? If yes could you help me to define which patch goes in witch repository ? Do you think some patches are required for both (upgrade process AND Note : it can be done later in a second time, if we face troubles with the current approach. |
|
Yes, I think it would be useful to split up the patches, but then again, the patches might conflict with one another (i.e both purposes requiring to overwrite the same method). Let's see: the core of the openupgrade_records override revolves around the And it looks like we can move this bit https://github.com/OCA/OpenUpgrade/blame/13.0/odoo/modules/loading.py#L215-L223 to a monkey patch of There's the logging of the xml records tagged onto this method: https://github.com/OCA/OpenUpgrade/blob/13.0/odoo/tools/convert.py#L255 And that's probably it for openupgrade_records. Methods like https://github.com/OCA/OpenUpgrade/blob/13.0/odoo/openupgrade/openupgrade_log.py#L9 and a couple from https://github.com/OCA/OpenUpgrade/blob/13.0/odoo/openupgrade/openupgrade_loading.py can be moved to that module when it's hosted separately. |
Well, Thinking back on it, I think it's not a blocking point. If we make two
So if a method is overloaded two times with similar or different patches, it's not a problem. (no conflicts) Pros
Cons
Even more, with that new approach, the port of the patches will be done in two separate times. When we want to port this repository to V15, we will first :
Regarding CI, we can exclude both incompatible modules |
|
I'll stop work on that PoC during a couple of days. In the meantime : Is it possible to have the point of view of other @OCA/openupgrade-maintainers ? mainly on that two points :
Other question : @sbidoul : do you have a "pypi" point of view ? ;-) |
|
Can we perhaps keep on working under oca/openupgrade, but with a radically different branch layout from 14.0 on as you describe? And I would propose openupgrade-analysis to host a renamed openupgrade_records with its own patches as layed out above. --edit-- Oh I see scripts are separated from the framework. In that case, it makes sense to move to openupgrade-framework, openupgrade-scripts and openupgrade-analysis |
|
I think that's a great initiative, thanks! |
|
14.0 branch in OCA/OpenUpgrade will have the new layout, with 4 folders:
But I totally agree with this approach and excited to have it. Thanks for catching what I had in my mind since a long and make it real. EDIT: @StefanRijnhart has finally said that separated, but I still think that better together. |
|
intermediate proposal : 1)
|
|
Please see my last comment. |
Yes we wrote in the same time !
I don't like to have subfolder, as it will generate extra work / custom configuration regarding CI, pre-commit, etc.., having module in the root directory is simplier.
👍
👍 again.
I still prefer my proposal ;-) (
This module :
On the contrary, for framework and migration script,
Thanks ! Also very excited ! I hope that this lighter project will bring in more contributors. |
|
The analysis module can not reside in any other repo like server-tools, because then the monkeypatches will affect everybody who's got that repo in their addons path. Thinking again, I think a separate repo for the analysis module would be nice and the rest can go in a single repo as far as I'm concerned (easy to update the module coverage in the same PR as the migration scripts). |
|
Oh, to be honest in the case of the analysis module the monkey patches can be activated in a post load hook. Would still be nice to have it separated so that it can be maintained/ported independently from the rest of the framework. |
|
I still think that everything can go together, as the same procedure for migrating a module (preserving commit history) can be applied here. The burden of maintaining other repo is not worth IMO. About tests, they can be perfectly done through different builds, and there's no problem on applying them even in the same repo. Anyway, we don't have any test right now, and mocking all the communication is a nonsense here, as it depends a lot on both versions. |
Hi @StefanRijnhart. That was true before, but not anymore, and since at least version 12. Something has been "fixed" somewhere between 8 & 12 version. I just discovered that point, working on that PR. See : Step to reproduce : Then : execute on a fresh created DB (with
Result : V8during step 1, the code is executed, just after the installation of the base module. V12Step 1 : no text is displayed. V14Same behaviour. Finally, with a new fresh database, in V14, if you make Result : the code is executed before the installation of am I wrong, or can we consider now that monkey patches are not dangerous in a module of a folder present in the addon path, if the module is not installed ? Edit : March, 17 2021Context :
Result :
Note :
|
Most likely yes! Sorry to be late to the party. This would be great and you have my support! 👍
It's an addon, and has possibly more usages outside of openupgrade... so it goes in addons repo. Simple. I like it. 👍
I had in mind 2 different strategies. First would be to just monkey-patch everything in If that approach doesn't work because it monkey-patches bottom-up, and instead it's absolutely needed to monkey-patch stuff before even the module loading system boots, then my 2nd approach would be to create a new script. Let's call it import click
import click_odoo
from click_odoo import odoo, OdooEnvironment
from unittest import mock
from contextlib import contextmanager
@contextmanager
def PatchedOdooEnvironment(*args, **kwargs):
"""Patch Odoo and then load its env.
Specific implementation might differ among versions. No need to have multiple branches.
""""
with mock("odoo.modules.loading", ...), mock(...):
yield OdooEnvironment(*args, **kwargs)
@click.command()
@click_odoo.env_options(environment_manager=PatchedOdooEnvironment)
@click.argument("upgrades_paths", type=click.Path(exists=True, readable=True, file_okay=False, dir_okay=True)
def main(env, upgrades_paths):
... #Run odoo update in the mocked env
if __name__ == '__main__':
main()Of course it could be done without click-odoo, but I think the commodities it supplies justify its usage. This would be a separate repo. Just a normal python package that you could install through pypi and wraps your current odoo installation, mocks it, and runs odoo in |
|
click-odoo is outside OCA and can't be used according OCA rules. And I personally don't want to have yet another library for this. About the module being in OCA/server-tools, it doesn't serve for anything except OpenUpgrade, and it will conflict for sure with the rest of the normal modules. I don't see why we have to make the effort to put it there while it can be kept here, everything together, but still, separated. |
If that can help we can propose it to OCA anytime. Let me know. |
|
Thanks @legalsylvain, looks like the monkeypatching problem is solved indeed. About click-odoo, if I understand correctly it implies installing odoo as a python package. Not everybody installs Odoo like that so I would prefer not to add that dependency. |
No it does not. All it needs is that |
|
Also about click-odoo not being OCA. I don't see such a big deal here. As Pedro said starting to host pure Python/non Odoo packages inside OCA repos may suck (because it is very different). But other than that, many OCA modules rely on pure Python packages hosted elsewhere, why would click-odoo be such a problem? |
|
Well, it's only a question of balance: what click-odoo brings as benefit to OpenUpgrade against having to deal with spread code across several places that can provoke conflicts, regressions, etc. Anyway, I'm in favor of moving click-odoo under the OCA umbrella (but for other things). We are using it on Dooba and it's an excellent work by Acsone. |
|
I've had the experience of scripting with click-odoo and without it, and put simply: it's better. 🤷♂️ It's just a library that makes odoo scripting more comfortable and I agree with @rvalyi. The same way we rely on In any case, before talking about the specific implementation details (remember that was just pseudocode), I think we should focus in the main subject: how to better inject the mocks? Current options are:
I guess option 3 would be best overall. I think options 1 and 2 would end up leading to some ugly workarounds sooner or later. Well... monkey patching is always ugly, but you get what I mean... |
|
I agree about using option 3, but not about putting that wrapper depending on click-odoo. Doing that, we will find that version X of such library doesn't work with OpenUpgrade by any reason. I prefer to encapsulate the needed code here. But all approaches as always have trade-offs. |
|
Hi @pedrobaeza
As said before, I think this module can be used in a non OpenUpgrade context Use case 1 A developper works on an 12.0 instance and wants to refactor fully a custom module. he realizes the job and then run the Use Case 2 Customer has an Odoo EE enterprise instance on premise with custom modules (that inherit
My assumption is that the current As said, there is no risk, regarding monkey patches.
@yajo :
Thanks !
I have no point of view regarding both strategies, and I don't use click-odoo ! so I abstain on that point, and I'll have to test click-odoo to understand your proposal. I'll be back in a couple of weeks ! ;-)
I just prefer to avoid a single file. for having ported the code from version 13 to version 14, it is really simple to have the same tree for official odoo project and for the patches bundle. Using meld is so very easy to check the differences. Anyway, thank you all for your enthusiasm ! |
|
OK, Sylvain, as you prefer, as you are the one pushing this, and I agree such use cases can apply. My only fear is the CI on that repo, but let's see. |
I'm probably not qualified to say anything here, but if option 1 is talking about a module that patches other modules I'll just say it sounds a bit scary. Option 3 is probably easier to understand for muggles :) |
|
I also think option 3 makes more sense. IMO both features could be implemented in a script (using I imagine something like a cli tool with these commands:
I don't see why these features should be packed in an odoo module, they're not meant to be deployed in prod/staging/dev databases afterall.. they're just tools |
|
@ivantodorovich for analysis, you can't do that, as it needs a very previous setup + 2 Odoo runnings (previous version and current one), so it must split from this "script", and also with the use cases depicted by Sylvain, you don't have the same previous setup. |
Well. For having tested this implementation. (still WIP)
A) pull a regular odoo (of the targeted branch) I mean that all people hosting an odoo can do that. Make open upgrades simple (as far as it's possible ...) AND accessible should be one of the objective of openupgrade maintainers. We have to keep in mind that openupgrade (migration scripts part) is for the time being, a project that is quite hard to sustain (in a financial point of view). Having more contributors and a simpler tool could bring in more people.
Yes, it's just one of the easy ways to deploy openupgrade framework, but this module doesn't have to be installed indeed. But again, I don't understand the other options ! so .. no problem if you think it's better ;-) Side question : @sbidoul : what is muggles ? deepl.com doesn't help me. |
|
https://en.wikipedia.org/wiki/Muggle - Harry Potter's freak terminology 😉 Thanks for the extra explanation. Then 1 seems very good with server_wide_modules declaration. I'd go definitively to that way, as it doesn't have extra burden. |
|
What do you think about adding an environment variable in openupgrade_framework? if os.environ.get('OPENUPGRADE_MONKEY_PATCH'):
BaseModel.load = _load
|
|
Thanks for clarifying @legalsylvain. I have become quite curious to look into click-odoo given all the praise that it receives in this thread, but if we can pull it off with vanilla Odoo (and I'm sure we can, speaking as someone who has ample experience with dirty monkeypatching in Odoo) that has its merits too. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
OK we can do a dumber poll. Just add a reaction to this message:
Edit note: I have hidden all comments related to the old poll, to keep this focused. Let's use this other one which is simpler and more secure. 😊 |
This comment has been minimized.
This comment has been minimized.
|
@legalsylvain's approach works very well. It has forced me to review all patched previously included in OpenUpgrade and I believe most of them could be dropped. I've been spending my corona quarantine on this task and I now have a working setup. openupgrade_records is moving to server-tools/upgrade_analysis here: OCA/server-tools#1941 I have proposed the following PRs onto Sylvain's branches:
I think we should proceed by pushing Sylvain's openupgrade_framework branch as oca/openupgrade/14.0 and move these PRs there. A small change in openupgradelib is necessary to run the migration
|
|
Thanks for your work guys, I've reviewed them all. |
|
Thanks @StefanRijnhart for this work, and the summary.
I don't know how to proceed, and I'm not comfortable with such tasks (creating new branch, initialize correctly OCA repo). Do I have to do something, or should I just wait until a 14.0 branch is available to make a new PR ?
I'm not sure to understand that point technically. But if I can help...
I created a dedicated issue to talk about the CI and the new design. #2476
I'll copy paste the old doc folder in the new branch at root level. Maybe some update should be applied, but I think it can be done in a second time, and we can talk about this point in a separate issue.
Regarding that point, i searched for a script that had generated the previous files (sample) but i did found any, in openupgrade project. |
|
|
@legalsylvain and I are proposing the OpenUpgrade framework and analysis into oca/openupgrade/14.0 here: #2482 |
|
Hi. I added the context in a note at the end of this comment here. As said, I don't think there is a negative impact on the openupgrade V14 design. CC : @StefanRijnhart regards. |
|
Thanks. That is indeed how I understand monkey patching to work. And you are right that it does not affect OpenUpgrade 14 because we use server_wide_modules. In upgrade_records we use the patching decorator to minimize impact, but it may be the case that a simultaneous upgrade of a different database is affected. That should be a rare situation though. |
|
@legalsylvain I think this issue can now be closed, no? |
|
Indeed ! |
As talked with @pedrobaeza here, it seems that the current design of the OpenUpgrade project has some limitation :
openupgrade_recordsmoduleI so had the idea, and it seems @pedrobaeza also ;-) to move out the specific things of openupgrade in a new repository, to have the possilibity to use odoo code + an extra repository.
After ~20hours of work, my PoC seems to work. (still wip). I decided to work on the 14.0 revision, as it seems that no work has been started.
Here is the current code : https://github.com/legalsylvain/openupgrade-framework/tree/14.0-upgrade-pocalypse (yes, I followed the Odoo SA convention, suffixing the name of the branch with -pocalypse)
Current design proposed.
openupgrade_recordsmodule that makes the same thing as before, calling another instance and producing
migration_analysis.txt. main changes :openupgrade_frameworka new module that contains all the patch made in Odoo. Using monkey patch. (simple exemple :
to avoid raising an error if view are broken : here.). Main changes :
meldto analyze all the differences.openupgrade_loading.pyandopenupgrade_log.py. (apply OCA convention (pre-commit & co).doc. (I think it should be located in another place). See original files.This specific module should not be installed, but added in the odoo.cfg file like this
server_wide_modules = openupgrade_framework. So all the monkey patches are correctly and early applied.openupgrade_scriptscontains all the script. (for the time being, only the
openupgrade_analysis.txtof a couple of modules)Total:
State of the work:
openupgrade_recordsworking, renamed intoupgrade_analysis: OK. PR to review here.TODO
move the repository under @OCA umbrella.create an empty branch 14.0 with classical CI.So, @pedrobaeza, @yajo : is this what you had in mind ?
@StefanRijnhart : what do you think regarding that move ?
thank you for your attention
The text was updated successfully, but these errors were encountered: