-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
OpenAPI: Define default response structure for ListOperations #21934
Conversation
Almost all Vault ListOperation responses have an identical response schema. Update the OpenAPI generator to know this, and remove a few instances where that standard response schema had been manually copy/pasted into place in individual endpoints.
See above two auto-referenced PRs:
|
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.
General commentary, but you could define the standard ListResponseWithInfo(...)
:
Lines 151 to 169 in 96bb634
// ListResponseWithInfo is used to format a response to a list operation and | |
// return the keys as well as a map with corresponding key info. | |
func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Response { | |
resp := ListResponse(keys) | |
keyInfoData := make(map[string]interface{}) | |
for _, key := range keys { | |
val, ok := keyInfo[key] | |
if ok { | |
keyInfoData[key] = val | |
} | |
} | |
if len(keyInfoData) > 0 { | |
resp.Data["key_info"] = keyInfoData | |
} | |
return resp | |
} |
This should get you a standard schema for a few other list endpoints, such as in SSH, PKI, and apparently some Core stuff too. :-)
However, each user of In this PR, I'd like to just cover what can be done centrally from I do see that it would be nice in future to add a helper function to abstract away the common parts of the structure created in |
@maxb said:
I don't disagree, but many of the interfaces simply use, e.g.: vault/builtin/logical/pki/path_fetch_issuers.go Lines 41 to 45 in 96bb634
which seems to be a generic schema? Or is what this endpoint is doing incorrect, and there shouldn't be a generic |
Yes, this is exactly what we need to happen, if generated client libraries are to be able to render responses into endpoint specific data structures, consistent with the rest of the initiative of documenting response structures. An additional consideration, is that there is currently no declarative specification of which endpoints actually return |
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, thanks for the contribution!
Actually, I just noticed there are some OpenAPI related test failures, e.g. ( |
Sorry about that... there are three PR checks which always fail on community PRs in this repo so I'm habituated to ignoring the red Xs :-( Fixed. |
We should really fix these sometime :) |
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 fixing the tests! 🎉
hashicorp/vault#21934 implements this centrally
* hashicorp/vault#21934 - a standard response structure is applied to all list operations that do not explicitly override it * hashicorp/vault#22043 - query parameters will now appear as method parameters in alphabetic order, rather than at the whims of Go map iteration order
Sync OpenAPI: StandardListResponse & sorted query parameters * hashicorp/vault#21934 - a standard response structure is applied to all list operations that do not explicitly override it * hashicorp/vault#22043 - query parameters will now appear as method parameters in alphabetic order, rather than at the whims of Go map iteration order
Almost all Vault ListOperation responses have an identical response
schema. Update the OpenAPI generator to know this, and remove a few
instances where that standard response schema had been manually
copy/pasted into place in individual endpoints.