datreeio / datree

Prevent Kubernetes misconfigurations from reaching production (again 😤 )! From code to cloud, Datree provides an E2E policy enforcement solution to run automatic checks for rule violations. See our docs: https://hub.datree.io
https://datree.io
Apache License 2.0
6.4k stars 359 forks source link

feat: issue#647 export log to file #938

Closed alonzyl closed 1 year ago

alonzyl commented 1 year ago

I would appreciate your feedback on whether there are any areas that I could enhance this cr. Any suggestions for improvement would be greatly appreciated.

while writing the code i came across a few questions about the flow, I would be happy to change anything if you think it would be better.

hadar-co commented 1 year ago

Hey @alonzyl :)

on validation should i try to write in the directory to check if there are righting rights? should all the test fail if the directory is not valid? Regarding these 2 questions, you should check if the directory exists and is writable. See here for some approaches to achieve this. If the dir does not exist or is not writable the export should be skipped without any other side-effects.

maybe not limit the file saving only to json, allow all supported output types. I think JSON is enough, it is flexible enough to cover most use-cases, if we see a need for more output types we can add them in the future:)

Thanks!

alonzyl commented 1 year ago

Hi, I updated the code with the comments above. Would like to hear you opinion :)

adifayer commented 1 year ago

@alonzyl Thank you for submitting this PR! 🚀 We will review it in the upcoming days :)

shalev007 commented 1 year ago

Hey @alonzyl thanks for the contribution! 🙏 looks simple and good however I think there's some extra code there, take a look at the comments

alonzyl commented 1 year ago

Hi, I totally agree with your remarks and have made the necessary changes accordingly. If there are more things that I can improve, I would be happy to make further changes :)

shalev007 commented 1 year ago

@alonzyl Thanks you so much for the changes, it looks great. merged 🥳