Fix saving option only using days argument

[thican]

Signed-off-by: Thibaud CANALE [email protected]

Description

The saving feature was unable to save content without the none default --days argument.
Now it should be able to store content with --start/--end and for the last day without --days argument, as described in the help section of arguments.

Checklist

• [Y] I have reviewed the contributing guidelines.
• [N] I have included unit tests for my changes or additions.
• [N] I have successfully run make test-docker with my changes.
• [N] I have updated the documentation.
• [Y] I have updated the changelog.

Questions or Comments

I was unable to test my commit because I met some issue to run the Docker test (I am behind a corporate proxy, I fails to fetch content from the WAN) or to execute directly the Python script (some circular imports issues).
About the documentation, this is a fix so there is nothing to update.

If someone could test for me, my tries were unsuccessful:

python3.9 elastalert/test_rule.py --config content/config/my_config.yaml --save-json content/data/my_data.json content/rules/my_rule.yaml

As you might have noticed, I store the timestamp in the class instead of querying for each call, this allows to keep a steady differential between days.

Best regards,

[jertel]

Thanks for the contribution! I understand the goal and it is a good idea to fix this. There is existing logic from lines 264 to 291 that should be moved into a re-usable function so that both run_elastalert and test_file are sharing the same start_time/end_time logic instead of implementing two slightly different code portions. That function should be where ts_now should be initialized, for modularity and re-use reasons.

Once a single function is created to handle the date calculations, a new unit test can be created to test that new function. I will enable the automated test runs for this PR so that you can begin getting test feedback, but it is expected that contributors can fully execute the unit tests locally before submitting PRs. Relying on GitHub to execute tests for each commit is not recommended.

[thican]

Hello @jertel, thanks for your review.

I agree with you, as it is also my point of view, there need to be a refactoring of those logics. However my (first) approach is to fix steps by steps stuffs which could be fixed without requiring to break or rework other part of the code.
I wasn't able to test my fix, but I hoped it could fix the issue I met with the --save-json option which was unable to behave correctly.

About the new function you talked about, did you plan to implement the bases of this new, refactored, logic?
I could also contribute more, but meanwhile I don't have enough knowledges on this project. Don't hesitate to contact me if you want.

Best regards,

[jertel]

Hello! No, I do not have plans to refactor this. Since you created the PR I am giving feedback to you so that the PR can be accepted. As it is now it will fix a bug but will also introduce technical debt to the project by having duplicated code. So my goal in providing the feedback to you is to help ensure the fix is correct, tested, and maintainable.

If you're unable to finish the PR we can leave the PR open for a while, and mark it as a Work In Progress (WIP) to see if anyone else would like to assist you.

[thican]

Hello again,

I submitted new patches to improve as much as I could without modifying code out of the scope of this issue.
I tried to run on my system make test, I am unsure how to interpret the results.

I am even thinking I over did however those modifications were required to not introduce new errors.

Is this the result you were expecting? Is it validate for you? Please keep me updated.

Best regards,

[jertel]

Yes, the changes look good. I only had the one code review concern about the timeframe param being required. Let me know your thoughts on that.

[thican]

Yes, the changes look good. I only had the one code review concern about the timeframe param being required. Let me know your thoughts on that.

I posted a comment explaining some details, pointing out timeframe isn’t required, now waiting for your review.
Thanks again for your review.

[jertel]

Thanks for the explanation; I see what you're doing now.