-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Binlogs Redacting support + Binlogs forward-compatibility reading support #9307
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
Binlogs Redacting support + Binlogs forward-compatibility reading support #9307
Conversation
rokonec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request change is only for missing that "backward-compatible-with" field.
Otherwise it looks OK. But I have to admit that I have not time to carefully review full extent of this PR.
|
Is there a design doc or some overview of what the redactor does - it sounds interesting? |
Hi @danmoseley, Design proposal of redactor is located here: https://github.com/JanKrivanek/MSBuildBinlogRedactor/blob/main/docs/DesignProposal.md (ultimate long term wishful plan is here https://github.com/dotnet/msbuild/blob/main/documentation/specs/proposed/security-metadata.md) |
ladipro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left comments inline, mostly nits.
src/Build/Logging/BinaryLogger/Postprocessing/TransparentReadStream.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/IBuildEventArgsReaderNotifications.cs
Outdated
Show resolved
Hide resolved
rokonec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
src/Build/Logging/BinaryLogger/Postprocessing/IBinlogReaderErrors.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/IBinlogReaderErrors.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/IBinlogReaderErrors.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/IBuildEventStringsReader.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/IBuildFileReader.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/ArchiveFileEventArgsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Build/Logging/BinaryLogger/Postprocessing/EmbeddedContentKind.cs
Outdated
Show resolved
Hide resolved
292072d to
a71f8b3
Compare
Fixes #9147, #9261 and contributes to #8400
Viewer complement PR: KirillOsenkov/MSBuildStructuredLog#732
Important
This contains all changes from
Context and changes are described in those individual PRs
Feel free to review everything together (this PR), or separately (above 2 PRs) - but not all 3 as you'd be viewing same changes twice
TBD:
localize all stringsDoneI'm contemplating adding support for recognizing over-reading and mismatched reading during events deserializationDone