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.

Sign up for GitHub

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

Jump to bottom

[Audit Logging] Custom audit logger parsing in xDS registry. #32970

Merged
merged 35 commits into from May 17, 2023
Merged

[Audit Logging] Custom audit logger parsing in xDS registry. #32970

merged 35 commits into from May 17, 2023

Conversation

rockspore
Copy link
Contributor

No description provided.

@rockspore rockspore added the release notes: no Indicates if PR should not be in release notes label Apr 28, 2023
gtcooke94
gtcooke94 approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, one small comment

src/core/ext/xds/xds_audit_logger_registry.cc Outdated Show resolved Hide resolved
markdroth
markdroth reviewed May 1, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment here -- the rest looks great!

src/core/ext/xds/xds_audit_logger_registry.cc Outdated Show resolved Hide resolved
@rockspore rockspore marked this pull request as draft May 1, 2023 22:53
@rockspore rockspore marked this pull request as ready for review May 16, 2023 17:04
@rockspore rockspore requested a review from markdroth May 16, 2023 17:05
markdroth
markdroth reviewed May 17, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nits remaining. Otherwise, this looks great!

src/core/ext/xds/xds_audit_logger_registry.cc Outdated Show resolved Hide resolved
src/core/ext/xds/xds_audit_logger_registry.cc Show resolved Hide resolved
@rockspore rockspore requested a review from markdroth May 17, 2023 14:28
@rockspore rockspore changed the title [Audit Logging] Custom audit logger parsing in xDS registry [Audit Logging] Custom audit logger parsing in xDS registry. May 17, 2023
markdroth
markdroth approved these changes May 17, 2023
View reviewed changes
src/core/ext/xds/xds_audit_logger_registry.cc Outdated Show resolved Hide resolved
@rockspore rockspore merged commit 05d5a04 into grpc:master May 17, 2023
66 of 67 checks passed
@rockspore rockspore deleted the xds-registry branch May 17, 2023 16:56
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 17, 2023
wanlin31 pushed a commit that referenced this pull request May 18, 2023
@rockspore @wanlin31
[Audit Logging] Custom audit logger parsing in xDS registry. (#32970)
26829a5
@yijiem yijiem added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels May 31, 2023
@erm-g erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023
@yijiem yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtcooke94 gtcooke94 gtcooke94 approved these changes

@markdroth markdroth markdroth approved these changes

@erm-g erm-g Awaiting requested review from erm-g

Assignees
No one assigned
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rockspore @gtcooke94 @markdroth @yijiem @erm-g