[REF] module_auto_update: Deprecate buggy features, add new API #1190

yajo · 2018-03-14T11:41:47Z

The previous implementation of this addon proved being extremely buggy:

It supplied out of the box a enabled cron to update Odoo that didn't restart the server, which possibly meant that upgrades broke things.
It overloaded standard Odoo upgrade methods that made i.e. installing an addon sometimes forced to upgrade all other addons in the database.
The checksum system wasn't smart enough, and some files that didn't need a module upgrade triggered the upgrade.
It was based on a dirhash library that was untested.
Some updates were not detected properly.
Storing a column into ir.module.module sometimes forbids uninstalling the addon.

For all of these reasons, the addon has been refactored:

All buggy features are now deprecated and moved into *_deprecated.* files.
A new update system has been added.
More tests.
New checksum system, tested too.
Smarter diff detection system that triggers only on really needed upgrades.
Cron is disabled by default.
Old installations should keep most functionality intact.

Most of the job is copied from @sbidoul's #1176, so credits are added where needed too.

lmignon

@yajo This PR MUST BE CLOSED!!!!!! It doesn't respect at all the ownership of the author and is a lack of courtesy regarding the work done by @sbidoul in #1176.

pedrobaeza · 2018-03-14T14:36:10Z

@lmignon why it doesn't respect ownership? Everything is properly credited, and he wants to save work to @sbidoul converging both needs, ours and yours. I don't see this as a discourtesy sincerely.

lmignon · 2018-03-14T15:05:45Z

@pedrobaeza In a lot of comments, @sbidoul explained his motivations for keeping the work he did in a new module. I fully share his point of view on this topic. This PR looks like an attempt to impose your way of seeing things regardless of the arguments expressed. Moreover, in #1176 the commit history is very clean and each commit is signed while here all the work done is summarized into one commit with @yajo as author. For me, this PR is disrespectful and does not respect the ownership of the work done. The name module_auto_update has nothing to do with the logic implemented in #1176.

pedrobaeza · 2018-03-14T15:16:19Z

Sorry, but you are the ones that want to make the things their way. This module is already in all existing versions, and you are forcing to use 2 modules and break possible installations imposing a new dependency. As you know, other things (some of mine for example) have been blocked because the same reason. The only thing for not forcing this was that we don't want you to load extra work, but now that we have made it, there's no excuse. I agree to rename the module in newer versions (v12), or even to try to put a link to make some kind of alias.

About the commits, if you insist, we can bring here that commits, and the final one will remove all and put it like it is now.

sbidoul · 2018-03-14T15:46:47Z

@yajo what you are doing here is extremely rude.

pedrobaeza · 2018-03-14T15:49:14Z

I don't agree, Stéphane. Check my previous comment.

sbidoul · 2018-03-14T16:45:35Z

Yes, commits matter, of course they do.

It's not you vs us. I was just expecting an answer to my last comment. Frankly all this deprecated stuff is so messy, for what benefit? Saving the uninstallation of one deprecated module. I just don't understand.

As said before, I don't care about the module name. So if you are ready to port and maintain all this from 8 to 11...

sbidoul

Please, at the very least, preserve my commits.

sbidoul · 2018-03-14T16:59:50Z

Can't get this thing out of my head :)

I try to put myself in your shoes, so:

with [9.0] add module_checksum_upgrade and refactor module_auto_update #1176 you will need to uninstall module_auto_update
with this PR you will need to remove system parameter module_auto_update.enable_deprecated

Is that correct?

yajo · 2018-03-15T09:33:04Z

Please, at the very least, preserve my commits.

Hi there pals, sorry if I offended somebody, 😞 that wasn't my intention at all! I just wanted to stop discussing and just get the job done.

When doing the job, I found myself on the situation on what to do with the commit history:

It didn't make sense to me to keep all the commits from [9.0] add module_checksum_upgrade and refactor module_auto_update #1176, since after all, if merged, we'd squash them in 1 or 2.
I could have had all those commits squashed and then one of mine over it moving all things into this addon, but then when following the next migration to other branches, @sbidoul's history would be lost also, because our git recommended method strips out all history from other folders, and module_checksum_upgrade would be one of them. At the end we'd have the same problem but on other branches, so I feel this wasn't a good solution.
I could have squashed all of this into the first @sbidoul's commit, and I have nothing against doing that (I can if you want), although I quickly remembered [10.0][FIX] help_online: Restore commit history and remove bug introduced at merge time web#647 (comment) and thought that attributing to @sbidoul some job he wasn't willing to do could be seen as a personal harm. As said above I didn't want to harm anybody here.

So, considering all points above, I decided to own the commit but credit @sbidoul in the README, in the commit message and in the copyright of the files. This is all open source software, so it's not actually wrong to copy and paste with the same license, but please keep in mind that my intention on the git history has been to find a good balance between being pragmatic and respectful. If you still want, I can squash all into a @sbidoul's commit; I'm open to suggestions since I consider @sbidoul the original designer of this code (I just did the duck-taping).

When thinking on this matter, please also consider these points:

Some code from module_checksum_upgrade was copy-pasted from module_auto_update too.
If we ever dumped module_auto_update as you suggested, all commit history from that addon would be lost in newer versions too.
The idea was completely borrowed from module_auto_update.
Having all of this in mind, @sbidoul also did his own commits in [9.0] add module_checksum_upgrade and refactor module_auto_update #1176 and added authorship and copyright wherever needed.
We didn't become angry because any of that 😉

Saving the uninstallation of one deprecated module. I just don't understand.

It would require quite a bit of manual job in our side that we want to avoid.

Besides, if the addon was broken, we think that it's better to fix it than to dump it.

At the end of the day, once the job is done, there's no more reason to split things.

As said before, I don't care about the module name. So if you are ready to port and maintain all this from 8 to 11...

Since Laslabs went away, we are almost the only maintainer of this code from 9-11, so obviously yes, we'll maintain it. Here we are deprecating some code that now will have reduced (if any) maintenance from us from now on, of course. Most of the new, undeprecated, code is yours, so I'm pretty sure you'll be ready to co-maintain it too, no matter the addon name. 😊

When backporting to v8, all deprecated options can be dropped, since there's no chance to break existing installations, and we just have to make sure it creates a module_auto_update.enable_deprecated parameter set to 0, so such instances don't need to care about the deprecated stuff when forward-migrating.

When forward-porting to future v12, it would be the same, except that we don't need to care about such parameter anymore.

with this PR you will need to remove system parameter module_auto_update.enable_deprecated

With this patch, this parameter is only set to 1 in the migration script (when upgrading to 9.0.2.0.0 or newer), which means that for new deployments, you simply don't need to care about the deprecated features.

In case you had an old deployment and want to disable them, you can set the parameter to 0 either before or after updating the addon. It will be respected.

lmignon · 2018-03-15T09:44:16Z

@yajo I'm very tired with your continuous squash by default of the commit histroy. If the history is proper and the author ask you to keep it, just keep it. I agree that sometimes we can squash some useless commits but if we take the time to write and organise our dev into proper and well structured commit, there's no reason to squash the history.

sbidoul · 2018-03-15T12:54:10Z

No hard feelings either, trust me.

Commits are important because it's all that remains over time. When we look at contribution history, it's easy to analyse commits, while it's hard to analyse unstructured code headers. And a well crafted commit history provides valuable information on the reason why things end up being how they are.

If we ever dumped module_auto_update as you suggested, all commit history from that addon would be lost in newer versions too.

That is false: the commits remain in the history of server-tools.

Some code from module_checksum_upgrade was copy-pasted from module_auto_update too.

That is true: a few tests and one method name. A tiny fraction.

I now give more prominent credit to laslabs for the idea, though.

Besides, if the addon was broken, we think that it's better to fix it than to dump it.

Absolutely, that's what I always do too. In this case however, it is not a fix, it is a full deprecation.

It would require quite a bit of manual job in our side that we want to avoid.

That is the only argument I see so far in favor of this PR over #1176. And IMO it is dwarfed by the additional complexity. If I understood it better I could change my mind. @yajo would you like to explain why uninstalling module_auto_update is more manual work for you, compared to changing a system parameter (module_auto_update.enable_deprecated)? In my understanding both are scriptable in a couple of lines of code.

yajo · 2018-03-15T13:34:46Z

module_auto_update is itself the addon that takes care of updating other addons. For that reason, a good design decision is to make it depend only on base. Otherwise, you easily get to the chicken-and-egg problem.

Basically with the next update, our automated systems would be calling env["base.module.upgrade"].upgrade_module() as usual, which lower on the stack would be calling some methods that wouldn't exist because they would be found in an uninstalled addon.

It's similar to some systems that provide a very basic base for others and need to vendorize libraries instead of relying on automatic dependency resolution systems. Pipenv is an example that comes to my mind.

Also, I know that's not your case, but some people still install addons with the git clone and symlink method. This update would break their flow too.

This is a very important piece in automated deployments and has to be treated with the care it deserves.

The deprecation process, however (adding the parameter), makes the addon work as it did before out of the box, breaking nothing. It lets the operators decide at which point and in which projects to stop using the deprecated features.

The deprecation diff wouldn't be so big were this the original mindset when proposing the patch, but since you fully rewrote the addon (and honestly it looks better), I think it's fair to use your code instead.

About the idea attribution, I'll comment in the other PR.

sbidoul · 2018-03-16T09:57:27Z

@yajo

It is true that module_auto_update will not upgrade itself automatically the first time when adding the dependency on module_checksum_upgrade. Its upgrade needs to be forced. That I can see as a real problem of #1176, although a very small one compared to the complexity of the deprecation approach, IMO.

Anyway if that's the only way to progress let's go.

Could you clarify the problem you see with clone+symlinks deployments?

Do you prefer to rebase this on top of my commits, or do you want me to do it?

yajo · 2018-03-16T10:47:34Z

Could you clarify the problem you see with clone+symlinks deployments?

Yep, the problem would be that, even if you updated module_auto_update before anything else, its update would fail due to depending on a new addon that is not in the addons path. You'd have to create the symlink before, and updates would get broken.

Do you prefer to rebase this on top of my commits, or do you want me to do it?

I don't want your commits to be lost in migrations due to what I explained in #1190 (comment) 😕, let me squash all into your 1st commit so you appear as the author. You deserve such recognition indeed! 🙌

sbidoul · 2018-03-16T10:54:01Z

Why don't you make 2 commits: one with my work and one with yours?

yajo · 2018-03-16T10:56:03Z

Hmm yes, that'd be better indeed, I'll do ASAP. 😉

…ated files - Files are clearly suffixed with `_deprecated` so we know those features have no support nor migrations. - Views are removed, since updating from UI was too buggy to support it anymore.

This code comes from the module_checksum_upgrade proposal at OCA#1176. * [ADD] module_checksum_upgrade It provides the core mechanism of module_auto_update without the cron nor any change to the standard upgrade mechanism. Instead it provides an API on which module_auto_update can build, as well as a method which can be called from a script to run the upgrade of modules for which the checksum has changed. * [IMP] refactor module_auto_update Make it depend on module_checksum_upgrade which provides the core mechanisms of managing the checksums. module_auto_update makes it automatic. * [IMP] module_checksum_upgrade: better exclusion mechanism Ignore files based on exclude patterns. Ignore uninstalled languages. Better default for patterns to ignore (*.pyc,*.pyo,*.pot,static/*) For better control on the hashing mechanism implement our own: it's quite easy, and the checksumdir module used previously had no test. * [MIG] module_auto_update: adapt to new checksum mechanism * [IMP] module_checksum_upgrade: raise in case of incomplete upgrade * [IMP] module_checksum_upgrade: improve default exclusion pattern * [IMP] module_checksum_upgrade: control translations overwrite * [IMP] module_checksum_upgrade: one more test * [IMP] module_checksum_upgrade: credits [ci skip]

yajo · 2018-03-16T12:32:22Z

OK, I have done it in 3 commits so everything is clear and still we have all history in the same addon, so we avoid losing your commits by accident in the future:

Moving all stuff into deprecated files.
Moving all your code into module_auto_update, pretty much verbatim. Your patches that were originally for this addon land now in the deprecated files.
My deprecation things.

pedrobaeza · 2018-03-16T13:14:27Z

if the changed module is base, Odoo treats update other way.

Not sure what you mean by "other way", AFAIK it just updates base and all of its dependencies as usual... Will check it though. confused

There was a commit implied when updating it.

yajo · 2018-03-16T14:03:25Z

I added a patch that makes the 1st autoupgrade since the module update know it needs to update all.

Since the sha algorightm has changed, this addon upgrade will mean the upgrade of every addons in the database. This seems a little bit bad, but after all it's not such a really big problem since it should happen only once, and it's not that uncommon.

sbidoul · 2018-03-16T14:02:43Z

module_auto_update/migrations/9.0.2.0.0/pre-migrate.py

+                      FROM ir_module_module
+                      WHERE checksum_installed""")
+    checksums = dict(env.cr.fetchall())
+    env["ir.module.module"]._save_checksums(checksums)


note the checksum algorithm has changed so this not help.

You are correct, I'll do a couple of upgrade tests just to be sure and remove it, thanks 😉

sbidoul · 2018-03-16T14:03:24Z

module_auto_update/models/module.py

+        Note: this method commits the current transaction at each important
+        step, it is therefore not intended to be run as part of a
+        larger transaction.
+        """


Raise an error here if enable_deprecated is set? I'm not sure the two methods work well together.

According to my tests, they work fine together.

sbidoul · 2018-03-16T14:06:39Z

module_auto_update/wizards/module_upgrade_deprecated.py

+            ])
+            if int(own_module.latest_version.split(".")[2]) < 2:
+                deprecated = "1"
+        return deprecated == "1"


headache 😄

yajo · 2018-03-20T13:18:42Z

Cool, this one should be ready to review.

Deprecated features get autoenabled only when upgrading
A warning is logged when you use deprecated thing
The deprecated system is based on the new one, so boty upgrade mechanisms work together flawlessly
Addon can now be uninstalled
I double-checked that updating an addon updates its dependencies
base is treated normally since the new sha algorithm should prevent many unneeded updates to it

sbidoul · 2018-03-20T13:32:59Z

module_auto_update/models/module.py

+        )
+
+        tools.config['overwrite_existing_translations'] = \
+            overwrite_existing_translations


This translation option may not work as expected. We are investigating.

OK, I'm gonna rebase all my commits in one, with another fix. Do you have push access to this PR? If so, you can push here the fix for that part after that 😉

My last functional tests show that overwrite_existing_translations works well. Our doubts came from an external factor. 😃

The previous implementation of this addon proved being extremely buggy: - It supplied out of the box a enabled cron to update Odoo that didn't restart the server, which possibly meant that upgrades broke things. - It overloaded standard Odoo upgrade methods that made i.e. installing an addon sometimes forced to upgrade all other addons in the database. - The checksum system wasn't smart enough, and some files that didn't need a module upgrade triggered the upgrade. - It was based on a dirhash library that was untested. - Some updates were not detected properly. - Storing a column into `ir.module.module` sometimes forbids uninstalling the addon. Thanks to Stéphane Bidoul (ACSONE), now we have new methods to perform the same work in a safer and more stable way. All I'm doing here is: - Cron is disabled by default. - Installed checksums are no longer saved at first install. - Old installations should keep most functionality intact thanks to the migration script. - Drop some duplicated tests. - Allow module uninstallation by pre-removing the fields from ir.mode.model. - When uninstalling the addon, the deprecated features will get removed for next installs always. Besides that, fixes for the new implementation too: - When uninstalling the addon, we remove the stored checksum data, so further installations work as if the addon was installed from scratch.

yajo · 2018-03-20T13:47:10Z

I squashed my commits so you can continue work on top of this; AFAICS my part should be ready to merge.

I patched another little thing: when uninstalling the addon, the stored checksums now disappear.

yajo · 2018-03-21T08:20:36Z

module_auto_update/models/module.py

+        )
+
+    @api.model
+    def upgrade_changed_checksum(self, overwrite_existing_translations=False):


Do you think it would be a good idea to add here a different default?: overwrite_existing_translations=os.environ.get("ODOO_I18N_OVERWRITE", False), so we can set this directly through an env variable in automated environments? 🤔

Yes. Not 100% sure about the ODOO_ prefix, but otherwise 👍 .

yajo · 2018-03-22T13:41:02Z

This one seems ready to merge.

pedrobaeza · 2018-03-22T13:45:56Z

Thanks to all!

- `l10n-spain` was never a good sample, since only Spanish projects would have it. Since Tecnativa/doodba#86 it's simply not needed in most cases. Devs can check the docs to know the structure. Thus, it can be removed from here. - `checksumdir` is not used anymore in `module_auto_update` since OCA/server-tools#1190. - `xlsxwriter` is included in Odoo 10+. - `requests` is included in Odoo 8+.

- `l10n-spain` was never a good sample, since only Spanish projects would have it. Since Tecnativa/doodba#86 it's simply not needed in most cases. Devs can check the docs to know the structure, and a vscode snippet has been added. Thus, it can be removed from here. - `checksumdir` is not used anymore in `module_auto_update` since OCA/server-tools#1190. - `xlsxwriter` is included in Odoo 10+. - `requests` is included in Odoo 8+.

Syncing from upstream OCA/server-tools (16.0)

yajo added enhancement needs review labels Mar 14, 2018

yajo added this to the 9.0 milestone Mar 14, 2018

yajo mentioned this pull request Mar 14, 2018

[9.0] add module_checksum_upgrade and refactor module_auto_update #1176

Closed

yajo self-assigned this Mar 14, 2018

lmignon requested changes Mar 14, 2018

View reviewed changes

sbidoul requested changes Mar 14, 2018

View reviewed changes

yajo force-pushed the 9.0-module_auto_update-refactor branch from f836adf to b3a2d32 Compare March 16, 2018 10:52

yajo force-pushed the 9.0-module_auto_update-refactor branch from b3a2d32 to d7e0266 Compare March 16, 2018 10:54

yajo force-pushed the 9.0-module_auto_update-refactor branch 2 times, most recently from ae4952b to fdfbe52 Compare March 16, 2018 11:23

yajo and others added 2 commits March 16, 2018 11:47

[REF] module_auto_update: Step 1, move all deprecated stuff to deprec…

82fb9a1

…ated files - Files are clearly suffixed with `_deprecated` so we know those features have no support nor migrations. - Views are removed, since updating from UI was too buggy to support it anymore.

yajo force-pushed the 9.0-module_auto_update-refactor branch from fdfbe52 to 78473b7 Compare March 16, 2018 12:29

sbidoul reviewed Mar 16, 2018

View reviewed changes

yajo force-pushed the 9.0-module_auto_update-refactor branch from 8c2b50c to 2d4a737 Compare March 20, 2018 13:11

yajo removed the work in progress label Mar 20, 2018

sbidoul reviewed Mar 20, 2018

View reviewed changes

yajo force-pushed the 9.0-module_auto_update-refactor branch from e3ffa9d to f2d10e1 Compare March 20, 2018 13:45

sbidoul approved these changes Mar 20, 2018

View reviewed changes

yajo commented Mar 21, 2018

View reviewed changes

carlosdauden approved these changes Mar 21, 2018

View reviewed changes

ThomasBinsfeld approved these changes Mar 22, 2018

View reviewed changes

yajo force-pushed the 9.0-module_auto_update-refactor branch from 8a3c57f to f2d10e1 Compare March 22, 2018 09:42

pedrobaeza merged commit 7d10384 into OCA:9.0 Mar 22, 2018

pedrobaeza deleted the 9.0-module_auto_update-refactor branch March 22, 2018 13:45

yajo mentioned this pull request Mar 22, 2018

New update mechanism landed in module_auto_update Tecnativa/doodba#130

Merged

lmignon mentioned this pull request Mar 27, 2018

[10.0] module_auto_update forward port 9.0 #1207

Merged

yajo mentioned this pull request May 16, 2018

[10.0][IMP] module_auto_update: Update module list before getting #960

Closed

yajo mentioned this pull request Feb 25, 2019

[DCK] Remove outdated samples Tecnativa/doodba-scaffolding#23

Merged

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#1190 from OCA/16.0

f978873

Syncing from upstream OCA/server-tools (16.0)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] module_auto_update: Deprecate buggy features, add new API #1190

[REF] module_auto_update: Deprecate buggy features, add new API #1190

yajo commented Mar 14, 2018

lmignon left a comment

pedrobaeza commented Mar 14, 2018 •

edited

Loading

lmignon commented Mar 14, 2018

pedrobaeza commented Mar 14, 2018

sbidoul commented Mar 14, 2018

pedrobaeza commented Mar 14, 2018

sbidoul commented Mar 14, 2018

sbidoul left a comment

sbidoul commented Mar 14, 2018

yajo commented Mar 15, 2018

lmignon commented Mar 15, 2018

sbidoul commented Mar 15, 2018

yajo commented Mar 15, 2018

sbidoul commented Mar 16, 2018

yajo commented Mar 16, 2018

sbidoul commented Mar 16, 2018

yajo commented Mar 16, 2018

yajo commented Mar 16, 2018

pedrobaeza commented Mar 16, 2018

yajo commented Mar 16, 2018 •

edited

Loading

sbidoul Mar 16, 2018 •

edited

Loading

yajo Mar 20, 2018

sbidoul Mar 16, 2018

yajo Mar 20, 2018

sbidoul Mar 16, 2018

yajo commented Mar 20, 2018

sbidoul Mar 20, 2018

yajo Mar 20, 2018

ThomasBinsfeld Mar 20, 2018

yajo commented Mar 20, 2018

yajo Mar 21, 2018

sbidoul Mar 21, 2018

yajo commented Mar 22, 2018

pedrobaeza commented Mar 22, 2018

[REF] module_auto_update: Deprecate buggy features, add new API #1190

[REF] module_auto_update: Deprecate buggy features, add new API #1190

Conversation

yajo commented Mar 14, 2018

lmignon left a comment

Choose a reason for hiding this comment

pedrobaeza commented Mar 14, 2018 • edited Loading

lmignon commented Mar 14, 2018

pedrobaeza commented Mar 14, 2018

sbidoul commented Mar 14, 2018

pedrobaeza commented Mar 14, 2018

sbidoul commented Mar 14, 2018

sbidoul left a comment

Choose a reason for hiding this comment

sbidoul commented Mar 14, 2018

yajo commented Mar 15, 2018

lmignon commented Mar 15, 2018

sbidoul commented Mar 15, 2018

yajo commented Mar 15, 2018

sbidoul commented Mar 16, 2018

yajo commented Mar 16, 2018

sbidoul commented Mar 16, 2018

yajo commented Mar 16, 2018

yajo commented Mar 16, 2018

pedrobaeza commented Mar 16, 2018

yajo commented Mar 16, 2018 • edited Loading

sbidoul Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Mar 22, 2018

pedrobaeza commented Mar 22, 2018

pedrobaeza commented Mar 14, 2018 •

edited

Loading

yajo commented Mar 16, 2018 •

edited

Loading

sbidoul Mar 16, 2018 •

edited

Loading