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

Fix for randomized retry interval format #624

Merged
merged 1 commit into from
May 18, 2024

Conversation

InigoGR
Copy link

@InigoGR InigoGR commented Feb 1, 2024

queue_job: fix retry format with tuple values

#348 added support for randomized retry intervals.
I could not set randomized retry intervals due to the formatting checks not being updated. This should fix it.

@OCA-git-bot
Copy link
Contributor

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

@InigoGR InigoGR marked this pull request as draft February 1, 2024 10:00
@InigoGR InigoGR marked this pull request as ready for review February 1, 2024 10:22
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Thanks for your contrib!
I guess it failed because we didn't have specific tests, right?
Any chance you add that? If you don't have time that's fine, but it's a really nice to have if you can 😉

Also, could you please rewrite the commit msg to something like:

queue_job: fix retry format with tuple values

#348 added support for randomized retry intervals.
I could not set randomized retry intervals due to the formatting checks not being updated. This should fix it.

What changes:

  1. includes module name
  2. includes the full description of the issue

queue_job/models/queue_job_function.py Show resolved Hide resolved
@InigoGR
Copy link
Author

InigoGR commented Feb 1, 2024

Thanks for your contrib! I guess it failed because we didn't have specific tests, right? Any chance you add that? If you don't have time that's fine, but it's a really nice to have if you can 😉

Also, could you please rewrite the commit msg to something like:

queue_job: fix retry format with tuple values

#348 added support for randomized retry intervals.
I could not set randomized retry intervals due to the formatting checks not being updated. This should fix it.

What changes:

  1. includes module name
  2. includes the full description of the issue

I will add test coverage for that case and change the commit message.

@InigoGR InigoGR marked this pull request as draft February 2, 2024 09:20
OCA#348 added support for randomized retry intervals.
Configuration of randomized retry intervals is not possible due to the formatting checks not being updated.
This should fix it.
@InigoGR
Copy link
Author

InigoGR commented Feb 5, 2024

Added some simple tests that should cover the changes in _check_retry_pattern and _parse_retry_pattern.

@InigoGR InigoGR marked this pull request as ready for review February 5, 2024 09:04
@InigoGR InigoGR requested a review from simahawk February 5, 2024 09:04
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Minor remark get a more clear exception. Pre-approving

Comment on lines 202 to +213
except ValueError:
raise exceptions.UserError(
record._retry_pattern_format_error_message()
)

def _retry_value_type_check(self, value):
if isinstance(value, (tuple, list)):
if len(value) != 2:
raise ValueError
[self._retry_value_type_check(element) for element in value]
return
int(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
except ValueError:
raise exceptions.UserError(
record._retry_pattern_format_error_message()
)
def _retry_value_type_check(self, value):
if isinstance(value, (tuple, list)):
if len(value) != 2:
raise ValueError
[self._retry_value_type_check(element) for element in value]
return
int(value)
except ValueError as exc:
raise exceptions.UserError(
record._retry_pattern_format_error_message()
) from exc
def _retry_value_type_check(self, value):
if isinstance(value, (tuple, list)):
if len(value) != 2:
raise ValueError("Random retry pattern must be a tuple with 2 digits")
[self._retry_value_type_check(element) for element in value]
return
int(value)

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@dreispt
Copy link
Member

dreispt commented May 2, 2024

@simahawk Is this good to merge?

@dreispt
Copy link
Member

dreispt commented May 18, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-624-by-dreispt-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7f55495 into OCA:14.0 May 18, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at cf35a44. 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.

6 participants