Closed henryx closed 1 year ago
The patch is OK with me. Do you have any ideas why CI actions are not run on this PR? All checks were run on old PRs (e.g.: https://github.com/jopenlibs/vault-java-driver/pull/17)
Do you see any issues at the: https://github.com/jopenlibs/vault-java-driver/blob/master/.github/workflows/gradle.yml ?
Hi @henryx.
I left some comments for the patch. Please fix ones when you have a time. Please take a look at the proposed fix style
Not clear for me: why there are only negative tests for revoke
and revokePrefix
?
Or are these not negative tests, but gaps with the implementation / test environment?
Sorry, one more comment:
I see all methods of Leases
except of Leases#renew
are expecting HTTP 204 (No Content) response. So, they cannot return anything. I guess the VaultResponse
object as the return value is bad design for this case.
What do you think about change this API to void
?
I think is not possible to return void
. Object VaultResponse
is an object that returns two data:
So, is possible to return VaultResponse(null, x)
, but I don't think it takes any advantages. However, I think can review my opinion, because in the future I want to port entire library to Java 11 and this code can be revised
@henryx thanks for contribution. The patch is merged. Do you need a hotfix release?
@tledkov yes, thank you!
According to tests, Leases lacks of a correct integration test suite. Also, leases implementation doesn't follow current Vault implementation (see https://github.com/hashicorp/vault/blob/main/CHANGELOG.md#080-august-9th-2017 and the Vault documentation). This MR provide to fix these lacks