-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Adding] Accountz monitoring endpoint and INFO monitoring req subject #1611
Conversation
Returned imports/exports are formated like jwt exports imports, even if they originating account is from config. Fixes #1604 Signed-off-by: Matthias Hanel <mh@synadia.com>
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.
Some comments about the json tags.
server/monitor.go
Outdated
|
||
type ExtExport struct { | ||
jwt.Export | ||
ApprovedAccounts []string `json:"approved_accounts"` |
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 and almost all of them in AccountInfo, you seem to not use ,omitempty
. Is there a reason? The boolean and int, if 0, could be omitted, which will make the JSON marshal smaller. Same for list of exports and imports (Exps, Imps, which by the way could probably be spelled out: Exports
and Imports
.
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 used omitempty sparingly because I imagine this to be used for debugging.
For this use case I imagine having values be explicitly listed makes more sense.
I used it only in cases where the source of the account makes a difference (jwt or config), which is also why I'll add it to this field.
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.
But I'll can add it everywhere. Your call.
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.
But if explicitly listed but equal to the type uninitialized value (false for boolean, 0 for numbers, etc), you would really not know the difference. Meaning if you see say "expired: false", I am not even sure what that would mean to be set explicitly.
Or here, approved_accounts
, if you don't see it it means it was nil. What difference (except for marshal result being bigger) would that make to get approved_accounts
received as []string{}
(or nil).
If you could give me an example of a case where having without omitempty
would help debug as opposed to if the field had omitempty
.
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.
Basically cut down on time spent trying to find out what should be there. If you don't think that matters I'll put omitempty on every field.
approved_accounts I added it already.
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.
LGTM
Returned imports/exports are formatted like jwt exports imports, even if they originating account is from config.
Fixes #1604
Signed-off-by: Matthias Hanel mh@synadia.com
I named the account specific subject INFO as it makes more sense than ACCOUNTZ.
ForCONNZ/SUBSZ we remain to have the plural even when scoped to the account, not so much for account itself.
Also setting update time on every update and account creation. (avoids empty values)
For consistency, setting incomplete when expired (dummy) account is created.
This is primarily to make the returned json make sense and look somewhat similar no matter the origin (jwt/cfg)