Skip to content
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

Fix in Leases section #18

Merged
merged 7 commits into from
Feb 13, 2023
Merged

Fix in Leases section #18

merged 7 commits into from
Feb 13, 2023

Conversation

henryx
Copy link
Collaborator

@henryx henryx commented Jan 7, 2023

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

@henryx henryx changed the title Leases fixes Fix in Leases section Jan 9, 2023
@tledkov
Copy link

tledkov commented Feb 10, 2023

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.: #17)

Do you see any issues at the: https://github.com/jopenlibs/vault-java-driver/blob/master/.github/workflows/gradle.yml ?

tledkov

This comment was marked as outdated.

@tledkov
Copy link

tledkov commented Feb 10, 2023

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?

@tledkov tledkov dismissed their stale review February 10, 2023 16:21

Hurry up. Some fixes are required

@tledkov
Copy link

tledkov commented Feb 10, 2023

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?

@henryx
Copy link
Collaborator Author

henryx commented Feb 11, 2023

I think is not possible to return void. Object VaultResponse is an object that returns two data:

  • REST response returned from Vault
  • Number of times that has tried to contact Vault

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

@tledkov tledkov merged commit e64e0e0 into jopenlibs:master Feb 13, 2023
@tledkov
Copy link

tledkov commented Feb 13, 2023

@henryx thanks for contribution. The patch is merged.
Do you need a hotfix release?

@henryx
Copy link
Collaborator Author

henryx commented Feb 14, 2023

@tledkov yes, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants