Skip to content

[16.0] queue_job: unify no delay option#521

Merged
OCA-git-bot merged 1 commit intoOCA:16.0from
camptocamp:16-qjob-fix-disable-key
Apr 2, 2023
Merged

[16.0] queue_job: unify no delay option#521
OCA-git-bot merged 1 commit intoOCA:16.0from
camptocamp:16-qjob-fix-disable-key

Conversation

@simahawk
Copy link
Copy Markdown
Contributor

@simahawk simahawk commented Feb 28, 2023

Current situation:

  • multiple keys for no good reason
  • half baked: not all of them used everywhere
  • no centralization
  • poor naming

With this change we'll have:

  • 1 and only one key to disable via ctx: queue_job__no_delay
  • 1 and only one key to disable via os env: QUEUE_JOB__NO_DELAY
  • backward compatibility with deprecation for old keys

@simahawk simahawk added this to the 16.0 milestone Feb 28, 2023
@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Feb 28, 2023

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

@simahawk simahawk force-pushed the 16-qjob-fix-disable-key branch from bef1eb4 to 5d03eaa Compare February 28, 2023 14:36
@simahawk
queue_job: unify no delay option
dff747c 
Current situation:

* multiple keys for no good reason
* half baked: not all of them used everywhere
* no centralization
* poor naming

With this change we'll have:

* 1 and only one key to disable via ctx: ``queue_job__no_delay``
* 1 and only one key to disable via os env: ``QUEUE_JOB__NO_DELAY``
* backward compatibility with deprecation for old keys
@simahawk simahawk force-pushed the 16-qjob-fix-disable-key branch from 5d03eaa to dff747c Compare February 28, 2023 14:38
grindtildeath
grindtildeath reviewed Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Is there any reason to use a double underscore in the name of ctx key and env var?
IMO it would be better to have a single underscore as in QUEUE_JOB_NO_DELAY to avoid potential troubles because it's not something that's easy to detect if you accidentally write only one.

@simahawk
Copy link
Copy Markdown
Contributor Author

simahawk commented Mar 1, 2023

Actually the env var has 2 underscores too. TBH this is just an approach I'm taking to identify keys by module especially if the are meant to be used in many many modules. So the __ is just to separate the name explicitly.
I can change that of course.

@guewen
Copy link
Copy Markdown
Member

guewen commented Apr 2, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Apr 2, 2023

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-521-by-guewen-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 589476b into OCA:16.0 Apr 2, 2023
@OCA-git-bot
Copy link
Copy Markdown
Contributor

OCA-git-bot commented Apr 2, 2023

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

@simahawk simahawk mentioned this pull request Sep 3, 2024
Merged
@thienvh332 thienvh332 mentioned this pull request Oct 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guewen guewen guewen approved these changes

+2 more reviewers

@grindtildeath grindtildeath grindtildeath left review comments

@theangryangel theangryangel theangryangel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved merged 🎉

Projects

None yet

Milestone

16.0

Development

Successfully merging this pull request may close these issues.

5 participants

@simahawk @OCA-git-bot @guewen @theangryangel @grindtildeath