[16.0][ADD] image_tag_environment: make server_environment an optional dependency - #575
Conversation
| "data": [ | ||
| "views/image_tag.xml", | ||
| ], | ||
| "auto_install": True, |
There was a problem hiding this comment.
Is it enough to get it automatically installed on environments where image_tag was already instealled and it only gets an update?
There was a problem hiding this comment.
I've added the migration script and it's safe enough. No issues in logs about fields removal or schema change.
7edd618 to
de71141
Compare
| [("name", "=", "image_tag_environment"), ("state", "=", "uninstalled")] | ||
| ) | ||
| if module: | ||
| module.button_install() |
There was a problem hiding this comment.
My experience also states that it is nice to get the data and add it to the right field. This happened to me and I would recommend you to do it this way 😉
There was a problem hiding this comment.
Which data? the one set in the environment?
The purpose of this script is to install the new split off module, so instances that were configured through the environment remain configurable through env -- by having the new *_environment module automatically installed
Users or sysadmins are expected to explicitly uninstall the *_environment module and re-configure their system.
To make that easier, we're working on this approach:
- [IMP] mail_environment: add uninstall hook server-env#262
- [IMP] server_environment: module uninstallation server-env#261
It's not done in this PR, though. At this stage we want at least to have the modules split.
There was a problem hiding this comment.
yes, but it will give you problems when you want to remove the environment, also, it happened to me that I decided to remove the dependancy as it was not required, at that moment, everything failed as the uninstall hook did not pass.... I am just warning, it shouldn't be a big deal to handle in the migration too.
There was a problem hiding this comment.
We don't need any uninstall hook here, because image_tag_environment doesn't actually override _server_env_fields
etobella
left a comment
There was a problem hiding this comment.
For me OK, just review my comment if possible 😉
|
This PR has the |
de71141 to
79975fc
Compare
|
@etobella for this split case we don't need any extra uninstall hooks or UserError exceptions. No field is namaged as server env field. |
This comment has been minimized.
This comment has been minimized.
lmignon
left a comment
There was a problem hiding this comment.
IMO image_tag_environment will never be migrated and disapear... I'm wondering why i put a dependency on server_environment 🤔 🙄
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at d5d6d6d. Thanks a lot for contributing to OCA. ❤️ |
PR is aimed to remove
server_environmentdependency fromimage_tagin the scope of this issue: