-
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
Added OpenAPI response structures for sys endpoints #18515
Added OpenAPI response structures for sys endpoints #18515
Conversation
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
…thub.com:hashicorp/vault into VAULT-12143/added_responses_for_sys_subsection_2
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.
think there's a few questions @averche should take a look at but lgtm
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
vault/logical_system_paths.go
Outdated
"keys_info": { | ||
Type: framework.TypeMap, | ||
Required: false, | ||
}, |
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 did not see this parameter anywhere in the response, not sure if I'm missing something.
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.
sorry is actually "key_info"
not keys... related line from handler.
Line 2840 in b741fa8
"key_info": nsScopedKeyInfo, |
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.
The code you referenced seems to only apply to PolicyTypeEGP
whereas the function deals with PolicyTypeACL
.
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.
Oh dang good catch.
vault/logical_system_paths.go
Outdated
"keys_info": { | ||
Type: framework.TypeMap, | ||
Required: false, | ||
}, |
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.
Same as above.
Co-authored-by: Anton Averchenkov <[email protected]>
Co-authored-by: Anton Averchenkov <[email protected]>
Co-authored-by: Anton Averchenkov <[email protected]>
…thub.com:hashicorp/vault into VAULT-12143/added_responses_for_sys_subsection_2
Co-authored-by: Anton Averchenkov <[email protected]>
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
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.
Please update other tests for the affected functionality (e.g. https://github.com/hashicorp/vault/blob/main/vault/external_tests/mfa/login_mfa_test.go for sys/mfa)
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
Before merging, please adjust the title & PR description (references to the RFC + related PR's would be great)
adding response objects for the sys endpoints laid out in VAULT-12143.