Cleanup script to search and remove rageshakes from applications based on a time #61
Conversation
For example, testing/demo mxids may want to be retained for longer.
michaelkaye
force-pushed
the
michaelk/script_to_remove_unwanted_rageshkes
branch
from
0c40cba
to
3f57c18
Compare
|
Have tried this in dry run mode on the server. and it is selecting the right rageshakes to delete. |
scripts/cleanup.py
Outdated
| app_name = parts[1].strip() | ||
| if parts[0] == "user_id": | ||
| mxid = parts[1].strip() | ||
| print(f"app_name {app_name} user_id {mxid}") |
There was a problem hiding this comment.
suggest moving everything other than the with gzip.open block outside the try/except.
There was a problem hiding this comment.
I'm not sure about this;
I was aiming for not taking any action (deletion etc) if the details.log.gz file was not found. Putting the remaining logic outside the try/except would mean it does get run, even if the (current) if block would always evaluate to False as app_name would always be None.
I feel it's more obvious if we simply don't evaluate the decision at all when we get a file not found error.
There was a problem hiding this comment.
Sorry, I failed to add: you'd want an early return False inside the except block.
The idea is to be clearer about which parts of the code we're expecting to throw. For example: currently we need to worry about what happens if _delete throws a FileNotFoundError. In that case, we'll report a misleading message about not being able to check the application name.
Generally, it's good practice to be minimal with the size of your try block; sticking extra code inside the try block as a way of skipping that code if the exception is throw means (a) you could end up catching exceptions you didn't want to; (b) it disrupts the flow of the code: the error handling for gzip.open is now a long way from that call.
Simple changes from codereview Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
scripts/cleanup.py
Outdated
| app_name = parts[1].strip() | ||
| if parts[0] == "user_id": | ||
| mxid = parts[1].strip() | ||
| print(f"app_name {app_name} user_id {mxid}") |
There was a problem hiding this comment.
Sorry, I failed to add: you'd want an early return False inside the except block.
The idea is to be clearer about which parts of the code we're expecting to throw. For example: currently we need to worry about what happens if _delete throws a FileNotFoundError. In that case, we'll report a misleading message about not being able to check the application name.
Generally, it's good practice to be minimal with the size of your try block; sticking extra code inside the try block as a way of skipping that code if the exception is throw means (a) you could end up catching exceptions you didn't want to; (b) it disrupts the flow of the code: the error handling for gzip.open is now a long way from that call.
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
| for l in args.limits: | ||
| parts = l.rsplit(":", 1) |
There was a problem hiding this comment.
l may be a poor name for a var due to the confusion with I and 1, but 🤷♂️
This script as a cronjob could be used to tidy up old rageshakes and implement #59