Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 22, 2023
Merged

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

merged 1 commit into from
Nov 22, 2023

Conversation

pcastelovigo
Copy link
Contributor

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
Copy link
Contributor

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

@pcastelovigo pcastelovigo changed the title [FIX][15.0] Allow use in multidb enviroments and list_db = false [15.0][FIX] Allow use in multidb enviroments and list_db = false Nov 20, 2023
@pcastelovigo
Copy link
Contributor Author

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

@guewen
Copy link
Member

guewen commented Nov 22, 2023

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
Copy link
Contributor Author

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
Copy link
Member

guewen commented Nov 22, 2023

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

@guewen
Copy link
Member

guewen commented Nov 22, 2023

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
Copy link
Contributor Author

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
Copy link
Member

guewen commented Nov 22, 2023

Many thanks for your work and understanding @pcastelovigo

@guewen
Copy link
Member

guewen commented Nov 22, 2023

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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 OCA-git-bot merged commit 93822bd into OCA:15.0 Nov 22, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants