[10.0] Revert "[ADD] queue_job module: migration from 9.0"

[lmignon]

Reverts #20
The queue_job addon doesn't depend on 'connector' and don't exist in 9.0.

ping @guewen

[coveralls]

Coverage remained the same at 77.94% when pulling 178526e on revert-20-10.0-mig_connector into 503e52f on 10.0.

[coveralls]

Coverage remained the same at 77.94% when pulling 178526e on revert-20-10.0-mig_connector into 503e52f on 10.0.

[guewen]

I agree. Fast-tracking. Besides, connector depends on queue_job so if queue_job is upgraded, connector is as well.

[pedrobaeza]

You are totally misunderstanding the use of migrations. This is not incorrect. The explanation is here: OCA/connector#256 (comment)

Don't fast track things that you don't know.

[guewen]

@pedrobaeza I fast-track things that might add a bug and that do not hurt to be reverted, until further proof that they work.

[pedrobaeza]

They don't hurt anything and it was correctly reviewed. Please restore it as it was.

[guewen]

@lmignon did you observed side effects?

[lmignon]

@pedrobaeza @guewen I did not observe side effects but queue_job don't depend on 'connector' therefore we must keep it without reference to the connector addon. Moreover we have successfully migrated 2 projects from 8.0 and 9.0 to 10.0 using the connector addon without this migration script. When you call odoo with -u all if a new dependency is detected it's automatically installed...

[pedrobaeza]

The problem is that you will have for example record rules duplicated, field definition garbage.... (although mostly working). This is the correct flow:

• Rename connector > queue_job in OpenUpgrade, and thus the rule, the models (queue.job, etc) are reassigned to queue_job module and not recreated. This includes custom translations and other things that people usually don't care about, but with this everything is smooth.
• On queue_job migration script, activate the upgrade of the connector module for not losing possible features on the migration. If you were finally only using queue features, you can remove the module connector.

[guewen]

Why would you have garbage? When a entry in ir.model.data does no longer exist in the module, it removes them during the upgrade.

And when you upgrade queue_job, if connector is installed, it will be upgraded as well, right?

Installing connector to remove it right after does not seem right to me.

[lmignon]

And when you upgrade queue_job, if connector is installed, it will be upgraded as well, right?

Yes it's.

Installing connector to remove it right after does not seem right to me.

👍

[pedrobaeza]

It's not installing, it's upgrading. Please, if you don't understand the migration process, don't refute the arguments. There's more than just ir.model.data records. Just look at update_module_names: https://github.com/OCA/openupgradelib/blob/1dc32e4f997e8f5e17e61a4a8ae5916ab24d2d2c/openupgradelib/openupgrade.py#L706

[lmignon]

@pedrobaeza PLZ don't say we don't understand the migration process. Here you introduce code required by the way how you migrate. This piece of code is only required if the migration is done by openupgrade. IMO this code should be in your migration script.

[pedrobaeza]

No, it's not my way of migration. It's the official OCA way. Any way, if you don't use OpenUpgrade, there isn't also any problem with this migration script.

[lmignon]

IMO we must avoid and it's not a good practice to add piece of code required by external tools not required by the addon itself.

[pedrobaeza]

Please stop trying to justify the incorrect revert, and just restore it.

It's not a bad practice. It's the correct one that it's being done since 8.0 in the OCA modules that needs any adaptation in the migration to major versions. One quick reference, but I don't want to lose more time with this:

• contract. OpenUpgrade part: https://github.com/OCA/OpenUpgrade/blob/98c001818f42f8baf74ce017968ebaf2eb30be72/openerp/addons/openupgrade_records/lib/apriori.py#L40 OCA module part: https://github.com/OCA/contract/tree/9.0/contract/migrations/9.0.1.0.0

[guewen]

@pedrobaeza do you realize that you sound rude and aggressive? I understand that you have been hurt by the revert, but honestly I don't think my actions are incorrect:

1. a member of the team says a merged PR could be an issue and propose a revert
2. I fast-track the revert, because I feel responsible with this repository and with the knowledge I had at this moment, I considered there was more risk to leave this code than to remove it. Never ever I said that we would not restore this code.
3. I ask for clarifications and explanation why this code is necessary and why it isn't a problem, for a potential restore.

Ultimately, my main concern is the part where you say that it would update the 'connector' module and you would need to remove it afterward. I won't accept that, maybe the original state of the connector module has to be checked.

[pedrobaeza]

Sorry for sounding that way.

I rephrase the sentence: "As in v10, queue features are now isolated in queue_job module, and if your dependent module(s) (for example, OCA/l10n-spain#619) only need that features and not connector features, you can uninstall connector module in your 10.0 installation. If not (or if you don't want), leave the module installed and it won't hurt."

[guewen]

I didn't had any explanation so far for this question:

And when you upgrade queue_job, if connector is installed, it will be upgraded as well, right?

There surely is a reason why it is needed for openupgrade. Please educate ourselves on the matter.

Here we are not in the openupgrade repository, so you have to assume that not everyone in this project will be familiar with it, and still they have to be responsible for it. As this change concerns only openupgrade and nevertheless you want to include it in this repo, at the very least you have to be sure that it has been well understood by the team, which require explanation / documentation. You can't blame someone for not knowing something that you don't explain.

[pedrobaeza]

Yeah, it will be upgraded also.

OK, I'm teaching you on this, but the facts are that:

• This doesn't break anything.
• You have overreacted and fast-tracked the revert PR without asking nor waiting any explanation on the author/reviewers.

[guewen]

Yeah, it will be upgraded also.

So we agree it's useless?

[pedrobaeza]

No, that's not what I'm saying. I understood bad the question. Without this, the migration won't work correctly. It's harmless for the rest.

[yvaucher]

This doesn't break anything.

IMO this is not enough to keep code. Untested / not properly commented code deserves to be removed. In such case it was really unclear this was of any use as it is related to an other project outside of this repository. While it was clearly not critical it had no reason to stay there.

The commit message is not clear enough nor the comments:
fe3fc86

@pedrobaeza it seems that #20 lacks some proper commit message / comment or explanation. There was not even one mention to OpenUpgrade when it was merged, so I think you overreacted and that #20 was merged too soon.

[simahawk]

I agree w/ Laurent, Guewen and Yannick.
There's no good reason to keep it if you don't explain why. And "why" cannot be "because openupgrade works like this" or "RTFM" :)
The link you provided just show a practice, not a bug/issue whatsoever.

[pedrobaeza]

No, it doesn't lack anything, as the comment is on code, not on commit (@guewen knows we prefer that method), and it wasn't merged too fast. OpenUpgrade has optimistic merging.

I'm very tired of this, and I don't understand why if you are not being able to give me any valid reason for not having this, you are still fighting against it. I have my flow covered even if you don't merge it. This is for the entire community. but I think all of you that don't make any OpenUpgrade contribution are not in position of determining what's the proper flow.

[pedrobaeza]

@simahawk, I have explained clearly why. Just read previous comments and links.

[pedrobaeza]

Do you need any other explanation?

[guewen]

Well, you explained that you need for openupgrade, but not why this code is required, i.e. why in odoo if you upgrade queue_job, connector is automatically upgraded and not in openupgrade. Is it not a bug in openupgrade?

[pedrobaeza]

Well, it has been explained previously and this is annoying, but here it is again:

• Reassigning of all the models, fields, security rules and so on is mandatory from connector module to queue_job module, or you will have garbage that is detected by database_cleanup, translations not respected and worst, a duplicated security rule and a duplicated security group without any of the user being correctly assigned. This is done in Merge connector into queue_job OpenUpgrade#1038
• As in v9 both queue and connector are the same module, you need to mark also connector to be upgraded here for keeping the module installed for any possible dependency that uses the connector features, not the queue features.
• If you don't need connector features in your DB after the migration, you can uninstall the module connector and keep queue_job. If you don't want, it's harmless.

[guewen]

Sorry @pedrobaeza I assure you that I read that and understood it clear and sound, but you keep repeating the same except that you don't answer to my question. So maybe I'm not giving enough details on my reasoning, I'll expand it there:

1. Do we agree that in a normal odoo process, when you -u an addon, the addons which depends on it are automatically updated as well?
2. So in this example, do we agree that in a normal odoo process, when you -u queue_job or -i queue_job, connector is upgraded as well?
3. Here comes my main question, then why does openupgrade not behave the same way? Why this code is necessary at all?
4. IMO it seems to be an error in the behavior of openupgrade if when you upgrade a module, the addons depending on it are not upgraded. Then if you fix that directly in openupgrade in an automatic way, then you should not need to do it in every modules with the same use case. It might be naive, but as nobody explained yet why openupgrade doesn't do it, I have no reason not to ask :)

[pedrobaeza]

Thanks for clarifying your doubts, but it's not a problem of upgrading, which OpenUgprade, as any regular Odoo distribution, does. It's a problem of preserving data:

• In v9, if you have for example 20 users with the permission "Connector Manager", without doing the renaming from connector to queue_job in OpenUpgrade, you will lose that on v10 and you will need to reassign the new group to that 20 users. Any way, I'm seeing that my colleague committed an error, as he forgot to rename also in this script the group XML-ID. This should be included:

openupgrade.rename_xmlids(
    env.cr, [
        ('queue_job.group_connector_manager',
         'queue_job.group_queue_job_manager',)
    ],
)

• Any custom translation for fields, models, views, etc will be lost.
• ir.model.data garbage will be generated.

[sbidoul]

@pedrobaeza regarding custom translations, won't we loose half of them anyway? The module is actually split, not merely renamed so I imagine a perfect migration cannot be achieved in this case.

[pedrobaeza]

Of course I know that not all the translations will be kept, but most of. Any way, that's the standard OpenUpgrade method: try to keep the most. Why are you so reluctant to add this?

[guewen]

@pedrobaeza I'll call me stubborn, but my whole point is there, you claim it's not a problem of upgrade but look:

With the migration file present, I run: odoo --workers=0 -u queue_job

2017-08-29 09:24:55,460 1 INFO ? odoo: Odoo version 10.0
2017-08-29 09:24:55,461 1 INFO ? odoo: Using configuration file at /opt/odoo/etc/odoo.cfg
2017-08-29 09:24:55,461 1 INFO ? odoo: addons paths: ['/data/odoo/addons/10.0', u'/opt/odoo/src/addons', u'/opt/odoo/external-src/connector', u'/opt/odoo/external-src/connector-ecommerce', u'/opt/odoo/external-src/sale-workflow', u'/opt/odoo/external-src/connector-magento', u'/opt/odoo/external-src/server-tools', u'/opt/odoo/external-src/bank-payment', u'/opt/odoo/external-src/queue', u'/opt/odoo/external-src/product-attribute', u'/opt/odoo/external-src/partner-contact', '/opt/odoo/src/odoo/addons']
2017-08-29 09:24:55,461 1 INFO ? odoo: database: odoo@db:5432
2017-08-29 09:24:55,486 1 INFO ? odoo.service.server: HTTP service (werkzeug) running on 0.0.0.0:8069
2017-08-29 09:24:55,488 1 INFO ? odoo.addons.queue_job.jobrunner: starting jobrunner thread (in threaded server)
2017-08-29 09:24:55,489 1 INFO ? odoo.addons.queue_job.jobrunner.channels: Configured channel: root(C:1,Q:0,R:0,F:0)
2017-08-29 09:24:55,501 1 INFO odoodb odoo.modules.loading: loading 1 modules...
2017-08-29 09:24:55,621 1 INFO odoodb odoo.modules.loading: 1 modules loaded in 0.12s, 0 queries
2017-08-29 09:24:55,693 1 INFO odoodb odoo.modules.loading: updating modules list
2017-08-29 09:24:57,583 1 INFO odoodb odoo.modules.loading: loading 55 modules...
2017-08-29 09:24:57,618 1 INFO ? odoo.addons.bus.models.bus: Bus.loop listen imbus on db postgres
2017-08-29 09:24:57,763 1 INFO odoodb odoo.addons.report.models.report: Will use the Wkhtmltopdf binary at /usr/local/bin/wkhtmltopdf
2017-08-29 09:24:57,824 1 INFO odoodb odoo.modules.migration: module queue_job: Running migration [>10.0.1.0.1] pre-migration
> /opt/odoo/external-src/queue/queue_job/migrations/10.0.1.0.1/pre-migration.py(7)migrate()
(Pdb) l 1,11
  1     def migrate(cr, version):
  2         if not version:
  3             return
  4         import pdb; pdb.set_trace()
  5         # In order to migrate connector from v. 9.0 to v. 10.0, we need to set
  6         # connector module state to 'to upgrade'
  7  ->     cr.execute("""
  8             UPDATE ir_module_module
  9             SET state='to upgrade'
 10             WHERE name='connector'
 11         """)
(Pdb) cr.execute("select state from ir_module_module where name = 'connector'")
(Pdb) pp cr.fetchall()
[(u'to upgrade',)]
(Pdb)

As you can see, the state of the connector is already to upgrade. So why this query UPDATE ir_module_module SET state='to upgrade' WHERE name='connector' is necessary at all? The state is already to upgrade, the query changes nothing at all!

[guewen]

Why are you so reluctant to add this?

If we finally find out this code is useless (as suggested by my previous comment), it will simplify all the upcoming migration for split of modules.
If we reach to the conclusion that this code is necessary despite what I show in the previous comment, hopefully, we should be able to tell why this UPDATE is required despite the fact that Odoo does it already, it can be documented for common knowledge or even better lead to a fix in openupgrade that would cover all the similar use cases.

(incidentally I don't like to merge unnecessary code or that I don't understand)

[pedrobaeza]

You are not coming from a 9.0 migration, and thus the process is different. Don't miss the step of OCA/OpenUpgrade#1038. As said, it's harmless if you are not migrating from v9, but needed for a proper v9 migration (but remember that there's one mistake right now and we need to include here also the other code).

[guewen]

Okay... I still don't know why openupgrade would not automatically set the state of dependent addons to to upgrade even when you come from 9.0... but well, I see I won't have my explanation from you. I personally would accept a PR for the openupgrade script, just be sure to mention what is it for. (but I'm not the only one who decide).

[pedrobaeza]

OK, here it is: #24

[sbidoul]

@pedrobaeza so if I follow correctly, can we say this is a bug / missing feature in OpenUpgrade which reveals itself when a renamed module is not actually renamed but split in pieces?

I'm not saying we can't merge #24, but like @guewen I like to get to the bottom of things and I still don't understand the root cause.

[pedrobaeza]

OK, I'm not going to follow with this. If you don't want it, I have my own branch for migrations.

[hbrunn]

wow, what a heated discussion. I'm a big fan of understanding stuff instead of just accepting someone claiming something, and @sbidoul's comment sounds a lot like that, so I'll try my best.

Some preliminaries:

• OpenUpgrade is a patched Odoo, so yes, it pulls new dependencies
• OpenUpgrade also deletes unclaimed xmlids on a module

What @pedrobaeza wants to avoid is that when you upgrade from 9 to 10, you'll lose all records created by connector(old) which now live in job_queue(new), because all the former connector.* xmlids that belong to job_queue.* now will be unclaimed after updating connector. Depending on constraints, things might actually even break before when during the update job_queue is installed which might create new records that conflict with their equivalent that was created by connector.

That's why we need to do this probably somewhat backwards feeling stunt in OpenUpgrade of first renaming connector to job_queue, but then pulling connector again during the update of job_queue.

Is this intelligible?

About the code itself: There might be some xmlids that this migration script should move back into connector in order to avoid the inverse of what I wrote above, but I don't know the internals of this code well enough to tell if that's the case or not.

[guewen]

I'm a big fan of understanding stuff instead of just accepting someone claiming something, and @sbidoul's comment sounds a lot like that, so I'll try my best.

Exactly that.

If I understand correctly what you say, OpenUpgrade tricks the upgrade by modifying the ir_module_module table's content itself during the upgrade, it somewhat inverses connector and queue_job, so that in the end, the record for connector is no longer in the to upgrade. This migration allows to put it back to to upgrade. I'm probably vague in my understanding but I now see why this is needed and it makes much more sense. Finally!

Thanks a lot for your explanation ⭐

[sbidoul]

@hbrunn that helps, thanks. So I gather the root cause is therefore the renaming in the OpenUpgrade script that is actually not a true renaming in this case but merely a split where xmlids of the old must be copied to the new yet preserved in the old. Does that make sense?

Would it make sense to have a kind of split_modules alongside the renamed_modules that would behave slightly differently to cover this case?

Again, I'm not against merging #24, nor am I saying that anything must be done now, but if we have a more robust solution on the horizon that would be nice!

[hbrunn]

@guewen @sbidoul glad I could help

This could be done more beautifully indeed, but we try to avoid patching odoo core as much as possible, so we don't modify the upgrade process a lot. If we'd introduce the splitting function, we'd need to put the knowledge about what belongs where somewhere centrally in OpenUpgrade (that's how the mechanics of odoo work), and this will probably become a biggish list for all of OCA. And I like the fact that (nearly) everything relevant for some module happens in its migration script.

Here's a completely different idea to handle those cases: If we duplicate the module to be split (or actually n-ify it with n being the amount of resulting modules) in OpenUpgrade such that on the start of the upgrade run, we have modules split1, split2, ... splitN each with all xmlids from the original module. Given Odoo doesn't delete records pointed to by another xmlid, the rest then should follow automatically: The framework will delete the superfluous xmlids, and if some xmlid isn't claimed by any of the split modules, the record will be deleted too. Then the migration scripts of the split modules would be quite simple and natural, as they'd only contains xmlids that were actually renamed. Next time we split a module, let's do it like this. But I think that's rather something for OpenUpgrade 11

[sbidoul]

💡 thanks @hbrunn !