OCA / queue

Asynchronous Job Queue
GNU Affero General Public License v3.0
175 stars 451 forks source link

[15.0][FIX] Allow use in multidb enviroments and list_db = false #580

Closed pcastelovigo closed 9 months ago

pcastelovigo commented 9 months ago

Update runner.py to work with multiDB & list_db = False

Applied exactly the same solution as #416 in V14, for some reason it became stalled I have it in production via patchfile and everything works as expected.

Its related to #58 and #379

Previous PRs closed to keep the commit list usable

OCA-git-bot commented 9 months ago

Hi @guewen, some modules you are maintaining are being modified, check this out!

pcastelovigo commented 9 months ago

@guewen If this solution is not appropriate, I will look for another one

guewen commented 9 months ago

Hi @pcastelovigo, thanks.

I think I need more details to understand why a setup would need this option. In what scenario do you have list_db = False and cannot provide the db_name list / or db_name is different? Can you also help me understand how you handle crons in your setup? That's the same logic and there is no option to provide a list of databases for crons?

pcastelovigo commented 9 months ago

Hi @pcastelovigo, thanks.

I think I need more details to understand why a setup would need this option. In what scenario do you have list_db = False > and cannot provide the db_name list / or db_name is different? Can you also help me understand how you handle crons in > your setup? That's the same logic and there is no option to provide a list of databases for crons?

My setup is unmodified OCA OCB Odoo 15, with these options in config file: dbfilter = ^%d$ list_db = False

Everything works OK, including cron jobs. Trying to use queue_job in combination with others OCA modules gives me AccessDeniedError exactly as #379 fixes.

I found #416 solution that provides two methods to provide manually a list of databases using queue_job module, at the same time that it does not interfere in any way with the operation that the module was having until now.

I managed to write unit test for it and PRed it so that I can continue synchronizing to the OCA repository and understanding that the modification is harmless, compatible with #379 and could help other people like comments in #416

If for some reason is not appropiate, I'll close PR and stick to patchfile

guewen commented 9 months ago

But then #379 was not never ported to this version, wouldn't be the better correction to apply the same fix?

guewen commented 9 months ago

I'm trying to understand the root cause of the problem, which seems the AccessDeniedError error. Then, adding a new option to configure databases seems like a work-around, and if #379 fixes the root cause, then that's much better in my book.

pcastelovigo commented 9 months ago

But then #379 was not never ported to this version, wouldn't be the better correction to apply the same fix?

hello again @guewen

I took #379 to 15.0 and adapted the unit test, so it passes all the checks. Also I discarded the workarround.

Hopefully this could be merged this time

guewen commented 9 months ago

Many thanks for your work and understanding @pcastelovigo

guewen commented 9 months ago

/ocabot merge patch

OCA-git-bot commented 9 months ago

This PR looks fantastic, let's merge it! Prepared branch 15.0-ocabot-merge-pr-580-by-guewen-bump-patch, awaiting test results.

OCA-git-bot commented 9 months ago

Congratulations, your PR was merged at 7ddbab8ac15a79df7e0bc9c92b9073a079900275. Thanks a lot for contributing to OCA. ❤️