feat: updated SystemIdentifier to have type instead of region #70
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
…ted api messages feat: updated SystemIdntifier to have type instead of region and related api messages
NamanBalaji
force-pushed
the
feat/api-changes-for-system-sepration
branch
from
5dcee4a to
0fabe40
Compare
| message SetSystemLabelsRequest { | ||
| string external_id = 1; | ||
| SystemIdentifier system_identifier = 1; | ||
| string region = 2; |
There was a problem hiding this comment.
should region move to SystemIdentifier?
There was a problem hiding this comment.
Idea is to have a clear sepration between "logical" and regional systems. SystemIdentifier currently serves the purpose of identifying the logical system so I don't think adding region to it makes sense.
|
|
||
| message UpdateSystemL1KeyClaimRequest { | ||
| string external_id = 1; | ||
| SystemIdentifier system_identifier = 1; |
There was a problem hiding this comment.
Do we need backwards compatibility for this ?
|
After applying this changes to the CMK Client there are some things that feel a little weird. Some operations such as Update use this new SystemIdentifier (externalID and Type) all on the same field, but ListSystems for example contains everything at the root level. Just for consistency could we pick one way of doing it and apply it elsewhere? |
gogro
left a comment
gogro
left a comment
There was a problem hiding this comment.
cannot break api right now
…er (#72) Co-authored-by: Naman Balaji <namanb487@gmail.com>
|
No description provided.