Extend access list event types to allow printing title #47704
Conversation
kopiczko
added
the
no-changelog
kopiczko
force-pushed
the
kopiczko/access-list-events-ux-improvements
branch
from
6567b53 to
0c3bf52
Compare
| // AccessList is common access list metadata. | ||
| AccessListMetadata AccessList = 4 [ | ||
| (gogoproto.nullable) = false, | ||
| (gogoproto.embed) = true, | ||
| (gogoproto.jsontag) = "" | ||
| ]; |
There was a problem hiding this comment.
I don't love it's after the Status field, but that way it's backward compatible. (this isn't the only thing I introduced here I don't love about the field ordering)
kopiczko
force-pushed
the
kopiczko/access-list-events-ux-improvements
branch
3 times, most recently
from
d312ac6 to
2382a0e
Compare
kopiczko
requested review from
tcsc,
flyinghermit and
smallinsky
and removed request for
avatus and
kiosion
public-teleport-github-review-bot
bot
removed request for
tcsc,
flyinghermit and
kiosion
| // AccessListMemberMetadata contains common metadata for access list member resource events. | ||
| message AccessListMemberMetadata { | ||
| // AccessListName is the name of the access list the members are being added to or removed from. | ||
| string AccessListName = 1 [(gogoproto.jsontag) = "access_list_name,omitempty"]; | ||
|
|
||
| // Members are all members affected by the access list membership change. | ||
| repeated AccessListMember Members = 2 [(gogoproto.jsontag) = "members,omitempty"]; | ||
|
|
||
| // AccessListTitle is a plain text short description of the access list the members are being added to or removed from. | ||
| string AccessListTitle = 3 [(gogoproto.jsontag) = "access_list_title,omitempty"]; |
There was a problem hiding this comment.
That's a bit strange to include AccessListTitle inline in all these events. You already have AccessListMetadata defined above - can we instead just include that metadata object in all relevant events? Then it will be easier to extend do.
There was a problem hiding this comment.
I looked at it differently. I thought all AccessList{verb} events would have AccessListMetadata AccessList field and all AccessListMember{verb} events AccessListMemberMetadata AccessListMember field.
I can add AccessListMetadata AccessList to AccessListMember{verb} if you prefer. Please let me know what you think.
| @@ -310,6 +319,9 @@ message AccessListReviewMetadata { | |||
|
|
|||
| // RemovedMembers are the members that were removed as part of the review. | |||
| repeated string RemovedMembers = 6 [(gogoproto.jsontag) = "removed_members,omitempty"]; | |||
|
|
|||
| // AccessListTitle is the title of the reviewed access list. | |||
| string AccessListTitle = 7 [(gogoproto.jsontag) = "access_list_title,omitempty"]; | |||
There was a problem hiding this comment.
Same comment as above - I don't think this should be included here, instead include AccessListMetadata where needed.
| // AccessListTitle is a plain text short description of the access list. | ||
| string AccessListTitle = 1 [(gogoproto.jsontag) = "access_list_title,omitempty"]; |
There was a problem hiding this comment.
Nit: "AccessList" prefix is redundant here.
| // AccessListTitle is a plain text short description of the access list. | |
| string AccessListTitle = 1 [(gogoproto.jsontag) = "access_list_title,omitempty"]; | |
| // Title is a plain text short description of the access list. | |
| string Title = 1 [(gogoproto.jsontag) = "access_list_title,omitempty"]; |
There was a problem hiding this comment.
The problem with that is that the generated types have embedded fields, e.g.:
// AccessListUpdate is emitted when an access list is updated.
type AccessListUpdate struct {
// Metadata is common event metadata
Metadata `protobuf:"bytes,1,opt,name=Metadata,proto3,embedded=Metadata" json:""`
// Resource is common resource event metadata
ResourceMetadata `protobuf:"bytes,2,opt,name=Resource,proto3,embedded=Resource" json:""`
// Status indicates whether the resource operation was successful.
Status `protobuf:"bytes,3,opt,name=Status,proto3,embedded=Status" json:""`
// AccessList is common access list metadata.
AccessListMetadata `protobuf:"bytes,4,opt,name=AccessList,proto3,embedded=AccessList" json:""`
XXX_NoUnkeyedLiteral struct{} `json:"-"`
XXX_unrecognized []byte `json:"-"`
XXX_sizecache int32 `json:"-"`
}so it is possible to:
var accessListUpdate AccessListUpdate
accessListUpdate.AccessListTitle // vs accessListUpdate.Title Again, please let me know if you'd still like to change it.
kopiczko
force-pushed
the
kopiczko/access-list-events-ux-improvements
branch
from
2382a0e to
5007216
Compare
Title is the human-friendly field while the name is basically an ID. To improve UX we want to print the title in the audit log.