Open jgreen117 opened 2 years ago
@jgreen117 - what is the status here?
@barakm I am waiting for a review of the code. I am not sure what other tests may be required
Closed incorrectly
@barakm Sorry I am getting a bit stuck here again. I am trying to check out the code and update it to remove the commented out lines. But I cannot get it work while some of the checks havent completed. Have you run into this issue before?
@jgreen117 Looks like something weird with the travis-ci integration. I am looking into it. However, I am not sure what is exactly the problem you are facing. You should not have any problem to check out this branch ('patch-1'), make some changes, add a commit and push it. The failed checks cannot stop you from updating your PR. What exactly is the command you are trying to run, and what is the error?
Closing and re-opening PR to test Travis integration.
Travis error message: "We are unable to start your build at this time. You exceeded the number of users allowed for your plan. Please review your plan details and follow the steps to resolution."
https://app.travis-ci.com/github/logzio/sawmill/pull_requests
@DanMelman - Time to move to something else.
@barakm okay the comments have been removed and the checks have passed let me know if there are any other changes I should make
@jgreen117 we are working on finishing the migration away from Travis. Once this is done, @DanMelman will help with the review.
Another important thing @jgreen117 - you did not write any test for your code.
If you had, it would have helped you realize that the current code does not do what you think it does.
You can use InConditionTest
as an example.
@DanMelman Sorry for the delay this is a bit of a side project so took me a bit before I had free time again
I think I figured out what you were saying. I definitely fixed an issue with the code, but I am not 100% sure it is what you were pointing out to me.
I updated the evaluate function so I am actually pulling the pulling the field from the doc and using its value. ipHigh and ipLow are unchanged as those really should have IP's manually in them. So the 3 ip's are converted to numbers and returns true if the value in ipTest is lower than ipHigh and higher than ipLow
` public boolean evaluate(Doc doc) {
if (!doc.hasField(ipTest, String.class)) return false;
String value = (doc.getField(this.ipTest));
long ipLo = 0;
try {
ipLo = ipToLong(InetAddress.getByName(ipLow));
} catch (UnknownHostException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
long ipHi = 0;
try {
ipHi = ipToLong(InetAddress.getByName(ipHigh));
} catch (UnknownHostException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
long ipToTest = 0;
try {
ipToTest = ipToLong(InetAddress.getByName(value));
} catch (UnknownHostException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
return ipToTest >= ipLo && ipToTest <= ipHi;
}
`
@jgreen117 sure, let's start by you fixing all the open comments (please comment/mark each fixed comment so its easier to follow). After pushing all your changes, ping me and i'll go over it again.
@DanMelman Okay I updated the comments with my changes.
@jgreen117 please add a test class before requesting review (see previous comment regarding this https://github.com/logzio/sawmill/pull/258#issuecomment-1061722919)
@DanMelman
I have added the Test. However I am not sure how to run it. Please let me know if there are other tests I should add to it. I am currently only testing if the IP is in the middle (true) or above or below the range (false)
@DanMelman
I have added the Test. However I am not sure how to run it. Please let me know if there are other tests I should add to it. I am currently only testing if the IP is in the middle (true) or above or below the range (false)
@jgreen117 check out the github checks, your last commit fails compilation.
After fixing the compilation, push again and you will see the tests running there as well. You can also run your test locally in the IDE (not familiar with eclipse that you mentioned you are using)
@DanMelman thanks fixed the issue, forgot I renamed the class in github
Great @jgreen117, please make sure you also add tests to the non-happy-flow cases: illegal input type/structure, non existing field, and so on..
@DanMelman Okay added some more tests let me know if I missed anything
@DanMelman Let me know if there anything else I need. Thanks again for all the help with this
Create a new condition processor to check if an IP address is within a range of two other Ip's
So there is a test ip, a low ip, and a high ip. If the test ip is in between those other ip's then the condition returns true