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

✨ [#33] Add reset_db_save_after option to config model #37

Merged
merged 4 commits into from
Jun 13, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Feb 19, 2024

closes #33

@stevenbal stevenbal marked this pull request as draft February 19, 2024 10:55
@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch 3 times, most recently from 1258568 to 4a8e7ce Compare February 19, 2024 11:16
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 97.39%. Comparing base (704b1ae) to head (2220b2d).

Files Patch % Lines
log_outgoing_requests/admin.py 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
- Coverage   97.75%   97.39%   -0.36%     
==========================================
  Files          21       22       +1     
  Lines         445      461      +16     
  Branches       57       58       +1     
==========================================
+ Hits          435      449      +14     
- Misses          6        9       +3     
+ Partials        4        3       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -4,8 +4,10 @@
from .tasks import reset_config


def schedule_config_reset():
reset_after = settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER
def schedule_config_reset(config):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how I can add the OutgoingRequestsLogConfig typehint here without the docs check failing, I tried it with if TYPE_CHECKING but that didn't work

Copy link
Member

Choose a reason for hiding this comment

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

from __future__ import annotations and then a conditional import tends to work usually, but instead, why not pass the self.reset_db_save_after so you can provide a typehint reset_after: int | None so you're only working in primitives :)

@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch from 4a8e7ce to a28bc72 Compare February 19, 2024 11:27
@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch from a28bc72 to 4fc3883 Compare February 19, 2024 11:46
@stevenbal stevenbal requested a review from alextreme February 19, 2024 13:17
@stevenbal stevenbal marked this pull request as ready for review February 23, 2024 15:16
@alextreme
Copy link
Member

Ziet er goed uit, wel graag een review van Sergei afwachten voor de zekerheid

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

There's also a disclaimer that you can only schedule tasks in the future as long as your broker remembers the tasks.

E.g. if you use Redis and you set the reset to 1 week, it's very likely that the key for that task has expired in redis before that moment arrives, so it will never be reset.

@@ -4,8 +4,10 @@
from .tasks import reset_config


def schedule_config_reset():
reset_after = settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER
def schedule_config_reset(config):
Copy link
Member

Choose a reason for hiding this comment

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

from __future__ import annotations and then a conditional import tends to work usually, but instead, why not pass the self.reset_db_save_after so you can provide a typehint reset_after: int | None so you're only working in primitives :)

Comment on lines 209 to 218
reset_db_save_after = models.PositiveIntegerField(
_("Reset saving logs in database after"),
null=True,
blank=True,
help_text=_(
"If configured, after the config has been updated, reset the database logging "
"after the specified number of minutes. Note: this overrides the "
"LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable."
),
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd also add a min value validator to set at least 1 minute, 0 minutes would immediately reset it and makes it unclear in the code if the value is set to 0 or None with the soft-boolean check if config.reset_db_save_after

Adding the validator removes ambiguity

@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch from 8f26f5b to 76809c0 Compare February 27, 2024 09:47
Comment on lines 218 to 220
"depending on the broker that is used, if this value is duration is too long "
"the key for the reset task might have expired before that time. So make sure not to "
"set too large a value for the reset."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"depending on the broker that is used, if this value is duration is too long "
"the key for the reset task might have expired before that time. So make sure not to "
"set too large a value for the reset."
"depending on the broker that is used, if this duration is too long "
"the key for the reset task might have expired before that time. So make sure not to "
"set too large a value for the reset."

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

couple more things to polish but otherwise seems good to merge

tests/test_tasks.py Outdated Show resolved Hide resolved
@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch from 76809c0 to 81bcc94 Compare February 27, 2024 10:37
@stevenbal stevenbal force-pushed the feature/33-configurable-reset-db-save-after branch from 81bcc94 to 2220b2d Compare February 27, 2024 10:38
@stevenbal stevenbal merged commit f4953c8 into main Jun 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to configure the duration before logging settings are reverted
4 participants