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

Classifier removed from labeler #109

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Classifier removed from labeler #109

merged 2 commits into from
Feb 10, 2022

Conversation

mazhurin
Copy link
Collaborator

@mazhurin mazhurin commented Feb 2, 2022

No description provided.

@mazhurin
Copy link
Collaborator Author

mazhurin commented Feb 9, 2022

The previous idea of labeling relied on the assumption that all the IPs active immediately before a massive attack are not malicious. We don't have much evidence to support this assumption.

@mazhurin mazhurin requested a review from mhqz February 9, 2022 18:48
@mhqz
Copy link

mhqz commented Feb 9, 2022

Hi @mazhurin, based on the description of the problem, and my still limited understanding of the system, the patch seems fine to me, thanks for sharing this 👍

From a general development perspective; I'd like to suggest (only if it adapts to the current workflow that you're following), to remove commented code as we can recover it from commits if needed.

I've also noticed that tests are failing from a while and I'd like to ask; are those UT something relevant to fix? If that's the case I could volunteer to fix them in the near future.

@mazhurin
Copy link
Collaborator Author

Thanks, @mhqz .

  • commented code: absolutely.
  • tests: yes, they are failing. We have a lot of outdated tests. We need to remove the old ones and disable PR merge if tests are failing. If you have time to do that, sure, it would be nice.

@mazhurin mazhurin merged commit 949c27d into develop Feb 10, 2022
@mhqz
Copy link

mhqz commented Feb 10, 2022

Thank you @mazhurin, I've created a separate issue for it

@mhqz mhqz mentioned this pull request Feb 10, 2022
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.

2 participants