-
Notifications
You must be signed in to change notification settings - Fork 100
ListResource: revisit handling of null ListResult
fields
#1167
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
Conversation
91cfa76
to
6075e3f
Compare
6075e3f
to
7dce0b7
Compare
ListResult
fields
62c8251
to
65651f6
Compare
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 👍🏻
Diagnostics: diag.Diagnostics{}, | ||
} | ||
} | ||
|
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.
Future consideration: Should we eventually add a NewListResultsWithDiag
so that you always create results from the request struct? 🤔
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.
Maybe! If it's a diagnostics-only response, then we have ListResultsStreamDiagnostics()
in this pull request. I did not put it on the request struct because (hand-waving) a diagnostics-only response does not deal in schema business.
Very open to 💡 to refine that API.
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.
🚀
Description
ListResult.{Identity,Resource}
can be "null" in a couple ways. This change handles both.It also updates diagnostic messages to be more user-actionable.
Rollback Plan
Changes to Security Controls
None