-
Notifications
You must be signed in to change notification settings - Fork 103
NIFIREG-234: Publish events for tenant CRUD #161
Conversation
Thanks for the contribution, @brosander. Will review... |
Thanks @kevdoran it seems like there are sporadic failures around some of the tests, that's why all the close/reopen, my other pr had seemingly unrelated test failures as well... |
Ex: https://travis-ci.org/apache/nifi-registry/jobs/504770097
During install of the assembly |
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 had a chance to pull this down and test it today. It is a nice addition of Event instrumentation for tenants. I had a couple minor comments that I am interested in other people's opinion on.
public static Event userCreated(final User user) { | ||
return new StandardEvent.Builder() | ||
.eventType(EventType.CREATE_USER) | ||
.addField(EventFieldName.USER, user.getIdentity()) |
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.
Tenant Events should probably have the identifier field as well.
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.
Good point, will add
COMMENT; | ||
|
||
USER_GROUP, | ||
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'n not sure we should reuse the EventFieldName.USER for the the user that was created. In other contexts it indicates the user that performed the operation (speaking of which, maybe we want that information for tenant CRUD events?)
I would suggest adding new EventFieldNames that align with the the tenant data model, such as TENANT_ID
and TENANT_IDENTITY
that can be used for all tenant types, or if we want to break it down into users and groups, USER_ID
, USER_IDENTITY
, USER_GROUP_ID
, USER_GROUP_IDENTITY
. What do you think?
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 think USER_ID, USER_IDENTITY, USER_GROUP_ID, USER_GROUP_IDENTITY more closely matches the terminology in object, methods of the tenant resource but don't have very strong feelings either way.
@kevdoran I think I've addressed the feedback, please let me know if I've missed anything or if further changes are needed |
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.
Thanks for making those changes, @brosander. +1, merging to master
No description provided.