[IMP] modules: skip autoinstall of unintalled modules #367
Conversation
|
|
|
upgradeci retry with only base iap_mail |
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
aec1a38 to
7464dda
Compare
|
upgradeci retry with always only base iap_mail |
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
7464dda to
4c2b786
Compare
aj-fuentes
left a comment
aj-fuentes
left a comment
There was a problem hiding this comment.
A first quick pass, some nits to keep the diff to the point :)
I will experiment with this a bit soon. Thanks!
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
4c2b786 to
64feb35
Compare
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
64feb35 to
888f583
Compare
| _assert_modules_exists(cr, module) | ||
| to_autoinstall = _get_autoinstallable_modules(cr, module) | ||
| if to_autoinstall: | ||
| if module in ENVIRON["__modules_to_skip_autoinstall"]: |
There was a problem hiding this comment.
This at the moment is not doing anything because the method trigger_auto_install is only called in the context of new_module and in some pre scripts also for new modules. But it does not hurt to keep it just in case, as the method can be called elsewhere
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
888f583 to
005e6a3
Compare
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
005e6a3 to
f319738
Compare
cawo-odoo
left a comment
cawo-odoo
left a comment
There was a problem hiding this comment.
LGTM and so did my test on upg-3468129
src/util/modules.py
Outdated
| si = ENVIRON["__modules_to_skip_autoinstall"] | ||
| if old in si and into in si: | ||
| si.discard(old) |
There was a problem hiding this comment.
Why the conditional removal?
It should always be removed, after the verification that into is an existing module, a few lines below.
There was a problem hiding this comment.
The condition should have been if old not in si and into in si: si.discard(into)
old | into | action
--------------+-------------+----------
unskipped | unskipped | nothing
unskipped | skipped | discard 'into' (if 'old' is installed?), 'into' should be installed if needed
skipped | unskipped | nothing, 'old' does not exist anymore but we can still discard it
skipped | skipped | keep 'into' in the skip list, 'old' doesnt matter
There was a problem hiding this comment.
We should discard old always. into should be kept only if old was as well in the list. In other words I agree with your condition if old not in si and into in si: si.discard(into)
In the case where an autoninstall module is uninstalled by users prior to the upgrade start, this module could be reinstalled during the upgrade if there were dependency changes and those dependencies get force installed. This improvement keeps track of any module that was elligible for autoinstall before the upgrade starts and skips its force installation when dependencies change.
jjmaksoud
force-pushed
the
master-skip-autoinstall-removed-modules-maji
branch
from
f319738 to
f040950
Compare
5 participants
In the case where an autoninstall module is uninstalled by users prior to the upgrade start, this module could be reinstalled during the upgrade if there were dependency changes and those dependencies get force installed. This improvement keeps track of any module that was illegible for autoinstall before the upgrade starts and skips its force installation when dependencies change.