[ADD] module_auto_update: Create module by hughesbm · Pull Request #882 · OCA/server-tools

hughesbm · 2017-06-29T21:01:14Z

Automate module updates by comparing module directory checksums.

Completes RFC #790

oca-clabot · 2017-06-29T21:01:17Z

Hey @hughesbm, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@hughesbm (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

lasley

Excellent first PR @hughesbm!

Comments inline

lasley · 2017-06-30T00:26:33Z

+    "version": "10.0.1.0.0",
+    "category": "Uncategorized",
+    "website": "https://odoo-community.org/",
+    'author': 'Brent Hughes, '


LasLabs if on the clock. You go in Contributors section of ReadMe using LasLabs email.

lasley · 2017-06-30T00:28:27Z

+    'author': 'Brent Hughes, '
+              'Juan José Scarafía, '
+              'Odoo Community Association (OCA)',
+    'license': 'AGPL-3',


relicense to LGPL-3, this was OK'd in the RFC #790 (comment)

lasley · 2017-06-30T00:28:46Z

@@ -0,0 +1,31 @@
+# -*- coding: utf-8 -*-
+# Copyright 2017 LasLabs Inc.
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).


LGPL, apply to all

lasley · 2017-06-30T00:28:56Z

@@ -0,0 +1,83 @@
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
+   :target: http://www.gnu.org/licenses/agpl-3.0-standalone.html
+   :alt: License: AGPL-3


lasley · 2017-06-30T00:28:59Z

@@ -0,0 +1,83 @@
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg


lasley · 2017-06-30T00:40:26Z

+    @api.depends('checksum_dir')
+    def post_upgrade_actions(self):
+        for r in self:
+            r.checksum_installed = r.checksum_dir


r.write({ 'checksum_installed': r.checksum_dir, 'upgrade_available': False, })

Can't this be into button_immediate_upgrade directly?

@yajo It's also called from upgrade_module in base.module.upgrade; button_immediate_upgrade isn't used for applying scheduled upgrades.

lasley · 2017-06-30T00:40:44Z

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+    <data>


can remove data tag

lasley · 2017-06-30T00:40:53Z

+            <field name="key2">client_action_multi</field>
+        </record>
+    </data>
+</odoo>


newline at EOF

lasley · 2017-06-30T00:40:58Z

@@ -0,0 +1,79 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+    <data>


lasley · 2017-06-30T00:41:11Z

+        mods = self.env['ir.module.module'].search(
+            [('state', '=', 'to upgrade')]
+        )
+


kill the spaces

Just noting this seems OK now.

yajo

Code OK (apart from @lasley's comments)

yajo · 2017-06-30T08:31:01Z

+
+class Module(models.Model):
+    _inherit = 'ir.module.module'
+    _rec_name = 'name'


You don't need this line.

yajo · 2017-06-30T08:31:16Z

@@ -0,0 +1,2 @@
+id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
+access_ir_module_module_user,ir_module_module user,model_ir_module_module,,1,0,0,0


Do you really need this file?

I guess not! Thanks! 👍

oca-clabot · 2017-06-30T19:54:03Z

Hey @hughesbm,
We acknowledge that the following users have signed our Contributor License Agreement:

@hughesbm

Appreciation of efforts,
OCA CLAbot

hughesbm · 2017-06-30T19:57:56Z

The requested changes have been addressed. I still need to add tests, however.

tafaRU · 2017-06-30T20:30:54Z

+
+from . import models
+from . import wizard
+from . import tests


This line is not needed as tests is automatically inspected by Odoo.

lasley · 2017-06-30T23:13:47Z

Ping @jjscarafia

yajo

I feel this needs a little more simplification.

yajo · 2017-07-03T07:15:53Z

+    "name": "Module Auto Update",
+    "summary": "Automatically update Odoo modules",
+    "version": "10.0.1.0.0",
+    "category": "Uncategorized",


I'd use Extra Tools

yajo · 2017-07-03T07:26:33Z

+        string="Current Module Checksum",
+        compute='_compute_checksum_dir',
+    )
+    upgrade_available = fields.Boolean(


Could we just set state=to upgrade?

yajo · 2017-07-03T07:26:57Z

+    @api.depends('checksum_dir')
+    def determine_upgradeability(self):
+        upgradeable_modules = self.search(
+            [('state', 'in', ['installed', 'to upgrade', 'to remove', ])]


If we use state to track upgradeable addons, then you should remove to upgrade from here.

yajo · 2017-07-03T07:27:44Z

+        self.update_list()
+        self.determine_upgradeability()
+
+    @api.depends('checksum_dir')


I'm not sure... does api.depends work if the method is not marked as a compute method on some field?

It does not; you can only edit computed fields with depends methods. They technically run, but the changes aren't stored to DB because you are in the cache layer.

Then this decorator makes no sense

Agreed - I was mainly just explaining for my newb 😉

yajo · 2017-07-03T07:27:57Z

+            'upgrade_available': True,
+        })
+
+    @api.depends('checksum_dir')


Same about depends here

yajo · 2017-07-03T07:31:23Z

+                    'view_mode': 'tree,form',
+                    'target': 'main',
+                    'context': '{"search_default_upgrade_available": 1}',
+                }


If we use the state as said above, could we move all the checksum computing into update_list(), so we don't have to create additional actions, menu items and methods? This way, if the admin uses the standard system to upgrade the addons list, he will always update their checksums too. It seems like what he'd expect to happen.

yajo · 2017-07-03T07:36:47Z

+    </record>
+
+    <record model="ir.cron" id="base_module_upgrade_cron">
+        <field name="name">Perform Scheduled Module Upgrades</field>


I see no sense on having 2 crons. Checking the upgrades without upgrading makes no sense, as upgrading without checking what's to upgrade neither. Besides, you could have a race condition where there are more than 1 cron workers. Please have a Python method that does all and run it in a single cron.

I'm good with automating the whole thing. Let's hide the Apply Scheduled Upgrades button in the UI then as well.

wouldn't it be better to extend that method and make the cron call it instead?

Ohhhh yes, I like that better. That way we can trigger a manual update without the cron

yajo · 2017-07-03T07:40:15Z

+    @api.multi
+    def upgrade_module(self):
+        mods = self.env['ir.module.module'].search(
+            [('state', '=', 'to upgrade')]


Add to install too?

yajo · 2017-07-03T07:43:00Z

+    @api.multi
+    def _compute_checksum_dir(self):
+        for r in self:
+            r.checksum_dir = dirhash(get_module_path(r.name), 'sha1')


One question... If we would have addons directory full of .pyc or .pyo files, and under a containerized frozen environment where those files are autoregenerated on each build, would that produce a different checksum? I guess it should be the same because if the pycompiled file is based on the same input python file, it should produce the same output, but did you check this? Or is it possible to ignore such files?

I think you're correct here Jairo. The pyc files contain a moddate header, which is the date of compilation as I understand it. Due to this, the hash can and will change when the file is recomputed, regardless of whether there were actually any changes.

Luckily checksumdir has an excluded_extension key, which interestingly enough has pyc in it as the example:

sha256hash = dirhash(directory, 'sha256', excluded_extensions=['pyc'])

IMO we should use an array declared at class level to determine the extensions that should be excluded. We could also use a method, but I think that's a bit overengineered

The simplest balance between simplicity and configurability is an unexisting ir.config_parameter with sane defaults:

exclude = self.env["ir.config_parameter"].get_param( "module_auto_update.checksum_excluded_extensions", "pyc,pyo").split(",")

This code with a little note in the configuration section of README should do the trick.

@hughesbm - here's an example of ir.config_param

Ummm but don't copy whatever the hell I did with the tab/space mixture there. Spaces4Lyfe!

Yeah, although you don't really need to provide a ir.config_param record if you add a fallback value as in the code I put above.

lasley · 2017-07-06T00:45:09Z

+        default=False,
+    )
+
+    @api.multi


@api.depends('name')

hughesbm · 2017-07-07T21:40:40Z

In addition to the requested changes, I made a fairly large change to how checksum_installed is stored. The strategy of overriding module upgrade methods to store the checksum completely ignored module installation; modules would be immediately scheduled for upgrade after installation.

To deal with this, this module now uses the @api.depends decorator to watch for module state changes, and recomputes checksum_installed when state changes to 'installed'. Cancellation of upgrades and uninstalls also changes the state to 'installed', however, so those methods were overridden to prevent undesired behavior.

yajo

The way you are using the computation of checksum_installed seems to go against the purpose of such kind of fields.

If you want to check when an addon's state changes to installed, but skip doing things under some situations, you should override write and create methods, and put something like this inside them:

def write(self, vals):
  if vals.get("state") == "installed" and not self.env.context.get("retain_checksum_installed"):
    for one in self:
      one.checksum_installed = one.checksum_dir
  return super(Class, self).write(vals)

Also, on the methods where you need to retain the installed checksum:

def medthod_X(self):
  return super(Class, self.with_context(retain_checksum_installed=True)).method_X()

This would follow the normal Odoo framework logic and hooks to make all work as expected and be written as expected 😉

yajo · 2017-07-10T07:41:52Z

-    upgrade_available = fields.Boolean(
-        default=False,
+    checksum_installed = fields.Char(
+        compute='_compute_checksum_installed',


This field must not be computed. It only has to be updated when addon gets updated too, but it is a normal char field.

yajo · 2017-07-10T07:44:17Z

+
+    @api.multi
+    def button_uninstall_cancel(self):
+        return self.retain_checksum_installed(


You don't need these kind of weird hacks if checksum_installed is not a computed field.

Yeah, this is a lot better. 😆 Thanks!

yajo · 2017-07-12T07:30:09Z

+    @api.model
+    def create(self, vals):
+        self._store_checksum_installed(vals)
+        return super(Module, self).create(vals)


First, create the record, and then call to result._store_checksum_installed(vals), or it will have no effect.

yajo · 2017-07-12T07:30:39Z

+    @api.multi
+    def write(self, vals):
+        self._store_checksum_installed(vals)
+        return super(Module, self).write(vals)


First write the record and then call the other method, or it could have undesired results.

yajo · 2017-07-12T07:31:23Z

-        for r in self:
-            if r.state == 'installed':
+    def _store_checksum_installed(self, vals):
+        if (vals.get('state') == 'installed' and


I'd also check "checksum_installed" not in vals, to avoid double writes if the field is being manually written.

yajo · 2017-07-12T07:31:50Z

+        mods = self.env['ir.module.module'].search(
+            [('state', '=', 'to upgrade')]
+        )
+


Just noting this seems OK now.

yajo · 2017-07-12T07:32:06Z

+    @api.multi
+    def button_immediate_upgrade(self):
+        res = super(Module, self).button_immediate_upgrade()
+


Just noting this seems OK now.

yajo · 2017-07-12T07:32:56Z

BTW, you have conflicts.

yajo · 2017-07-17T07:44:09Z

+                    r.checksum_installed = r.checksum_dir
+            elif vals.get('state') == 'uninstalled':
+                for r in self:
+                    r.checksum_installed = ''


Set it to False, which is the standard in Odoo when we have an empty field.

self.write({'checksum_installed': False}) and kill the loop

yajo · 2017-07-17T07:45:41Z

+    checksum_dir = fields.Char(
+        compute='_compute_checksum_dir',
+    )
+    checksum_installed = fields.Char()


I feel this one should be read only, mostly now that you are exposing it in the UI.
You should also add string and help attributes to both fields for the same reason, to let the user know what's this.

Makes sense. Does exposing these fields in the UI have value, or should I take it out again? I keep wavering on this.

The value would be for the human. If a human cares, expose it - otherwise don't. In this case, I'd say don't.

lasley · 2017-07-07T22:08:18Z

-#. Navigate to Settings / Technical / Automation / Scheduled Actions
-#. Click "Check for Module Upgrades" or "Perform Scheduled Module Upgrades."
-#. Click "Edit", enter desired changes, then click "Save."
+This module will ignore .pyc and .pyo file extensions by default. To modify this, create a module_auto_update.checksum_excluded_extensions system parameter with the desired extensions.


with the desired extensions listed as comma separated values.

lasley · 2017-07-17T17:13:37Z

+    checksum_dir = fields.Char(
+        compute='_compute_checksum_dir',
+    )
+    checksum_installed = fields.Char()


The value would be for the human. If a human cares, expose it - otherwise don't. In this case, I'd say don't.

lasley · 2017-07-17T17:14:29Z

+
+
+def post_init_hook(cr, registry):
+    env = api.Environment(cr, SUPERUSER_ID, {})


This isn't tested. Also I don't think it's implemented correctly, because coverage should have a hit regardless of test status for hooks.

lasley · 2017-07-17T17:15:39Z

+    def _compute_checksum_dir(self):
+        exclude = self.env["ir.config_parameter"].get_param(
+            "module_auto_update.checksum_excluded_extensions",
+            "pyc,pyo"


trailing comma

lasley · 2017-07-17T17:15:42Z

+            r.checksum_dir = dirhash(
+                get_module_path(r.name),
+                'sha1',
+                excluded_extensions=exclude


trailing comma

lasley · 2017-07-17T17:17:48Z

+    def button_uninstall_cancel(self):
+        return super(
+            Module,
+            self.with_context(retain_checksum_installed=True)


trailing comma

lasley · 2017-07-17T17:17:52Z

+    def button_upgrade_cancel(self):
+        return super(
+            Module,
+            self.with_context(retain_checksum_installed=True)


trailing comma

lasley · 2017-07-17T17:18:43Z

+        upgradeable_modules = installed_modules.filtered(
+            lambda r: r.checksum_dir != r.checksum_installed
+        )
+        upgradeable_modules.write({'state': "to upgrade", })


no trailing comma. The point of the trailing comma is to allow for an option to be added without editing multiple lines in the diff. In the instance of a single line, this is a null point

lasley · 2017-07-17T17:18:55Z

+# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
+
+from . import test_module
+from . import test_module_upgrade


Newline at EOF

lasley · 2017-07-17T17:19:21Z

+    def upgrade_module_cancel(self):
+        return super(
+            ModuleUpgrade,
+            self.with_context(retain_checksum_installed=True)


trailing comma

hughesbm · 2017-07-17T23:14:08Z

Runbot is currently failing when creating the odoo instance, not due to changes here as far as I can tell.

lasley · 2017-07-17T23:37:03Z

Runbot fail due to OCA/maintainer-quality-tools#464

lasley · 2017-07-18T17:51:26Z

just wondering if we could get rid of 505af1e and put everything in a single commit

Agreed - this is pretty much a rewrite at this point. Only a bit of boilerplate was carried over from what I can tell.

hughesbm · 2017-07-18T18:47:40Z

Everything is now in a single commit. Thanks for the fantastic code reviews! 😄

yajo · 2017-07-19T07:22:06Z

+        'views/module_views.xml',
+        'data/cron_data.xml',
+    ],
+    'auto_install': True,


I was just about to merge, but saw this. Please do not set it to autoinstall. Also, add dependency on base module.

Thanks! I have made the requested changes and squashed.

Apparently those changes weren't as consequence-free as I expected and a test is now failing. Working on it now.

* Add checksum_dir and checksum_installed fields to ir.module.module * Add checksum_dir to compute current checksum of module directory in addons path * Add checksum_installed to store checksum of module directory when module was last installed or upgraded * Use checksumdir Python library to compute module directory sha1 hashes, ignoring pyc and pyo extensions * Extend update_list method to compare modules' checksum_dir and checksum_installed, then change state of modules with differing checksums to 'to upgrade' * Replace Apps/Updates menu item with menu item of same name, which updates apps list and displays tree view of ir.module.module records with state 'to upgrade' * Extend create and write methods to store computed checksum_dir as checksum_installed during module installation and upgrade, and set checksum_installed to False on uninstall * Use context to stop checksum_installed from being updated during upgrade/uninstall cancellation * Add cron job to periodically check for module upgrades by comparing checksums, then perform any available upgrades * Extend upgrade_module method (called by cron and 'Apply Scheduled Upgrades' menu item) to call update_list * Add post_init_hook to store checksum_installed of existing modules * Add test coverage

* Use dummy module to test update_list method instead of module_auto_update

hughesbm · 2017-07-19T09:49:11Z

@yajo The changes you requested are done, and I fixed the test that broke as a result. I'll clean up the commits after approval. Sorry about the mess, I got too hasty trying to wrap this up quickly.

yajo

Great! That's what I love about good tests like yours, they fail whey they must! 😉

lasley · 2017-07-20T14:09:31Z

Congrats on your first module Brent! Not very often your first one gets to be this fun or popular!

Yenthe666 · 2017-07-20T15:04:27Z

Hooray! I'll test this next week too. Kudos!

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch 2 times, most recently from 3e111b3 to 0c83b8e Compare June 29, 2017 23:19

lasley added the work in progress label Jun 30, 2017

lasley added this to the 10.0 milestone Jun 30, 2017

lasley suggested changes Jun 30, 2017

View reviewed changes

yajo approved these changes Jun 30, 2017

View reviewed changes

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch from 44bdb3d to 3ac2baf Compare June 30, 2017 20:06

tafaRU reviewed Jun 30, 2017

View reviewed changes

lasley mentioned this pull request Jun 30, 2017

[RFC] module_auto_update: Community Module Updater #790

Closed

yajo requested changes Jul 3, 2017

View reviewed changes

lasley reviewed Jul 6, 2017

View reviewed changes

yajo requested changes Jul 10, 2017

View reviewed changes

yajo requested changes Jul 12, 2017

View reviewed changes

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch from b9e621e to 9b3d6ee Compare July 13, 2017 19:44

yajo requested changes Jul 17, 2017

View reviewed changes

lasley suggested changes Jul 17, 2017

View reviewed changes

lasley added needs review and removed work in progress labels Jul 17, 2017

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch from a8d59bd to e4009a5 Compare July 17, 2017 22:39

lasley approved these changes Jul 18, 2017

View reviewed changes

elicoidal approved these changes Jul 18, 2017

View reviewed changes

moylop260 approved these changes Jul 18, 2017

View reviewed changes

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch from e4009a5 to e81ea1f Compare July 18, 2017 18:26

yajo requested changes Jul 19, 2017

View reviewed changes

hughesbm force-pushed the feature/LABS-418-create-module_auto_update branch from e81ea1f to fc1f809 Compare July 19, 2017 08:33

[FIX] module_auto_update: Fix test broken by changes

9ca903f

* Use dummy module to test update_list method instead of module_auto_update

yajo approved these changes Jul 20, 2017

View reviewed changes

yajo merged commit c62a744 into OCA:10.0 Jul 20, 2017

lasley deleted the feature/LABS-418-create-module_auto_update branch July 20, 2017 14:08

yajo mentioned this pull request Aug 1, 2017

[MIG][module_auto_update] Backport from v10 #918

Merged

yajo mentioned this pull request Mar 15, 2018

[9.0] add module_checksum_upgrade and refactor module_auto_update #1176

Closed

		@@ -0,0 +1,83 @@
		.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg

		@@ -0,0 +1,2 @@
		id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
		access_ir_module_module_user,ir_module_module user,model_ir_module_module,,1,0,0,0



		def post_init_hook(cr, registry):
		env = api.Environment(cr, SUPERUSER_ID, {})

Uh oh!

Uh oh!

Conversation

hughesbm commented Jun 29, 2017 • edited by lasley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oca-clabot commented Jun 29, 2017

Uh oh!

lasley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oca-clabot commented Jun 30, 2017

Uh oh!

hughesbm commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Jun 30, 2017

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hughesbm commented Jun 29, 2017 •

edited by lasley

Loading