Create IpCompareCondition.java #258
Conversation
|
@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 |
| e.printStackTrace(); | ||
| } | ||
|
|
||
| //System.out.println(ipToTest+" "+ipTest+" test"); |
There was a problem hiding this comment.
please remove commented out log lines.
|
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. |
|
Closing and re-opening PR to test Travis integration. |
|
Travis error message: 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 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. |
| if (!doc.hasField(ipTest, String.class)) return false; | ||
| String value = (doc.getField(this.ipTest)); |
There was a problem hiding this comment.
Added these two lines to check that ipTest is a field in the doc
If it is then I pull the value in that field
Otherwise return false
sawmill-core/src/main/java/io/logz/sawmill/conditions/IpCompareCondition.java
Show resolved
Hide resolved
sawmill-core/src/main/java/io/logz/sawmill/conditions/IpCompareCondition.java
Show resolved
Hide resolved
|
@DanMelman Okay I updated the comments with my changes. |
|
|
||
| private String ipHigh; | ||
| private String ipLow; | ||
| private String ipTest; |
There was a problem hiding this comment.
This is no really the ip to test, this is the name of field containing the ip to test
so I would just call it field
|
@jgreen117 please add a test class before requesting review (see previous comment regarding this #258 (comment)) |
|
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. |
@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. |
|
@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 |
sawmill-core/src/main/java/io/logz/sawmill/conditions/IpCompareCondition.java
Outdated
Show resolved
Hide resolved
| // TODO Auto-generated catch block | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
@jgreen117 you still have those auto generated TO-DO parts, please go over the code once again and clean it up to spare another reviewing cycle.
| @Override | ||
| public boolean evaluate(Doc doc) { | ||
|
|
||
| if (!doc.hasField(field, String.class)) return false; |
There was a problem hiding this comment.
indentation seems off, please do command+optional+L on this whole file to arrange it
|
|
||
| public class IpComparatorConditionTest { | ||
|
|
||
| public ConditionParser conditionParser; |
There was a problem hiding this comment.
indentation off, please do command+option+L before commiting
…eCondition.java Co-authored-by: DanMelman <33516253+DanMelman@users.noreply.github.com>
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