Skip to content
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

Rolling auditto configuration #121

Closed
wants to merge 6 commits into from

Conversation

maciejw
Copy link
Contributor

@maciejw maciejw commented Dec 4, 2019

this PR addresses #113

@nblumhardt I think, I'll stop here, take a look

I did few things

  • 7b14346 - it was to painful to scroll and find not obsolete methods, now latest overloads are at the end in separate files.
  • 2ee7bb5 - I notices order of parameters in docs was not correct.
  • e41ae31 - this is change of public API
  • aef8b04 - I extracted IO operations to separate class, just like Clock
  • d8e0098 - this is change in implementation and tests

after this PR I want to propose few changes to internal implementation, to make this code more manageable:

  • extract creation of FileSink and SharedFileSink form RollingFileSink, this will also remove some constructor parameters
  • inject PathRoller into RollingFileSink as a dependency this will be needed in next point
  • extract retention policy method from RollingFileSink, this also will remove few parameters from constructor
  • it would be nice to be consistent and open files in sinks constructor, now FileSink and SharedFileSink do it in constructor and RollingFileSink does it in Emit
  • after this RollingFileSink will have only parameters that it needs and will be easier to understand and optimize further, one of a few changes that comes to mind is consolidation of parameters just like we did with rollOnFileSizeLimit and fileSizeLimitBytes in public contract. :)

I hope this PR is not too big.

@nblumhardt
Copy link
Member

Thanks! Organisation of the commits is good :-)

There's quite a bit to digest here, it'll take some time to review.

I've tried re-queuing the PR build, not sure what broke in there ....

@maciejw
Copy link
Contributor Author

maciejw commented Dec 6, 2019

take your time

this build is related to sourcelink github support, I'll let you know if I figure out something with it... there is an update to beta version of this package, https://www.nuget.org/packages/Microsoft.SourceLink.GitHub/1.0.0 I'll try to build it in WSL and see if it can be reproduced and fixed, and I'll make separate PR in case of success :)

@maciejw maciejw force-pushed the rolling-auditto-configuration branch from d8e0098 to 8d60484 Compare December 16, 2019 03:07
@nblumhardt
Copy link
Member

Howdy! I spotted more commits coming to this one so I left it to bake, guessing you're at another point we might review again, @maciejw ? I'll do my best to get 👀 on it if so :-) thanks!

@maciejw
Copy link
Contributor Author

maciejw commented Mar 4, 2020

@nblumhardt hello, all work besides conflicts is done on my part, of course I welcome feedback and I'll apply any code changes you desire... for now, I'll resolve conflicts and we can try to move with this forward...

@nblumhardt
Copy link
Member

Great, thanks for the update, @maciejw 👍

@nblumhardt
Copy link
Member

Hi @maciejw - doesn't look like we managed to move forward with this one; if you're still keen to push ahead, please let me know, just closing for housekeeping purposes :)

@nblumhardt nblumhardt closed this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants