-
Notifications
You must be signed in to change notification settings - Fork 310
intelmqctl: On stop wait longer for bots to be stopped #2598
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
Conversation
the 3.10 pytest fail is unrelated (see #2602) |
I put quite some comments in there. Looking at the other code, I see much less comments. If I should shrink the comments just let me know. |
Please not, inline comments are great :) |
Alright then. The tests (that shall pass) pass. Any additional feedback? (also notify if not, so I can squash the commits into one commit) |
Is there something still blocking the merge? (please don't feel pressured, I just want to avoid this got lost in the whole fixing the pytest pipelines issue) |
Friendly reminder for this PR. |
A code review and testing (+ changelog entry, but that's minor). Most of us do that work honorary, so it's sometimes hard to find the time. |
Alright 👍 I know finding times in the free time for such things is always hard. Personally I also always say contributing to OSS is not supposed to make your live more stressfull, instead it is supposed to be fun. So I didn't want to bother you, it's really mostly about avoiding this PR getting lost (I know that edge is sometimes sharp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks OK now, thanks.
But your PR introduces a bug:
Expected behaviour:
intelmq@localhost:~> intelmqctl -t json stop
{
"deduplicator-expert": "disabled",
"feodo-tracker-browse-collector": "disabled",
"feodo-tracker-browse-parser": "stopped",
"file-output": "stopped",
"postgres-output": "stopped",
"spamhaus-drop-collector": "disabled",
"spamhaus-drop-parser": "stopped",
"taxonomy-expert": "stopped"
}
Current behaviour:
intelmq@localhost:~> intelmqctl -t json stop
{}
Oh yes. In the last iteration I removed the update to (I'm only updating the |
retry multiple times on `intelmqctl stop` to check if bots really stopped, since the bots might take longer to stop. Using retry in constrast to increasing the sleep_time keeps the delay short in case the bots did already stop.
Co-authored-by: Sebastian <[email protected]>
rebased on develop and added a changelog entry |
Thank you for your contribution and especially for your patience! The patch will be included in the next IntelMQ release coming end of week. |
Thanks for reviewing and improving the quality of the contribution. Having the changes so soon (with the new release, so without installing from some other branch or so) is also quite nice 👍 |
fixes #2595
retry multiple times on
intelmqctl stop
to check if bots really stopped, since the bots might take longer to stop. Using retry in constrast to increasing the sleep_time keeps the delay short in case the bots did already stop.Note: I intend to make at least the amount of retries performed configurable so that if the current total
sleep_time
of 4.75 seconds does not suffice for some users they are able to increase it.