Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
authz: End2End test for AuditLogger #6304
authz: End2End test for AuditLogger #6304
Changes from 2 commits
5778913
cebf932
d405ab2
0baf0e6
b569c67
135565a
c19e304
1e7fcc4
e5991f2
620d990
cba398c
de59ce5
9a8ee49
36bc552
b255385
7777c5b
191fdf6
80d38b4
46fda00
a02960d
56eba6e
cb01a30
4f30f15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Now that we have EventContent, do we still need this field singled out?
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'm checking EventContent only once but I'd still prefer to check SPIFFE every time
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.
Is there any specific reason behind that? I thought if Spiffe ID is populated in one case, we can safely assume it's already there.
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.
Why not
s.EventContent = event
and make this field type*audit.Event
? (Also consider a rename tolastEvent
?)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.
Here EventContent is a map created at
loggerBuilder
but manipulated bystatAuditLogger
. At the end I'm usingloggerBuilder
to access it's content. I'm not sure how to achieve the same using*audit.Event
- maybe only if I use a slice or map wrapping it?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 DON"T UNDERSTAND THIS> Why can't you just change the type of this field (now
lastEventContent
) to*audit.Event
and rename the field tolastEvent
? Are you saying the tricky bit is communicating between the test and the constructed logger? If so, 2 options:*audit.Event
and*s.lastEvent = *event
**audit.Event
and*s.lastEvent = event
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.
Wow what happened with my last comment here?? Sometimes the shift key sticks on my keyboard, but I can't believe I didn't see it. Sorry if that came off as rude, I didn't mean to type in all-caps.
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.
No worries!
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.
This needs to check the error.
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'm leaning towards ignoring the errors here - #6304 (comment)