-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Add option to add IP address to existing list #13586
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
|
I've added a basic test for this. |
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.
@barryvdh in general your changes looks good, but static tests failing due to Cyclomatic Complexity of 10 in tests. Could you adjust your test to have less complexity or at least suppress the warning?
|
@ihor-sviziev I tried to follow the existing test, but should I just create my own seperate test for this, instead of using the provider? |
61aab30 to
5747bb3
Compare
|
@ihor-sviziev I've updated the tests. All checks are passing now. |
|
Hi @ihor-sviziev, thank you for the review. |
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.
Can you please remove the comma sign between IP-addresses on the line? #L152. I believe that this will help.
The test fails with:
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Set exempt IP-addresses: 127.0.0.1, 127.0.0.2\n
+'Set exempt IP-addresses: 127.0.0.1 127.0.0.2\n
Please, fix the test and push again.
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.
Thanks, that was changed since I made this PR. I've updated it now.
|
Hi @ihor-sviziev, thank you for the review. |
Description
When adding a new IP-address, you have to copy the existing ones. This adds a
--addflag to just append the addresses, instead of replacingManual testing scenarios