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
Add DevServices support for Vault extension #17083
Conversation
@vsevel Please review! |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building be82634
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment✖ |
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 made some comments.
And I missed integration tests.
...ns/vault/deployment/src/main/java/io/quarkus/vault/deployment/DevServicesVaultProcessor.java
Show resolved
Hide resolved
...ns/vault/deployment/src/main/java/io/quarkus/vault/deployment/DevServicesVaultProcessor.java
Show resolved
Hide resolved
extensions/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/DevServicesConfig.java
Show resolved
Hide resolved
be82634
to
f572a1f
Compare
@machi1990 rebased |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f572a1f
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 Linux #📦 integration-tests/gradle✖ ✖ ✖ ✖ ✖ ⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 extensions/vertx-http/deployment✖ |
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.
not familiar with dev services. I have only minor points.
...ns/vault/deployment/src/main/java/io/quarkus/vault/deployment/DevServicesVaultProcessor.java
Show resolved
Hide resolved
...ns/vault/deployment/src/main/java/io/quarkus/vault/deployment/DevServicesVaultProcessor.java
Outdated
Show resolved
Hide resolved
extensions/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/DevServicesConfig.java
Outdated
Show resolved
Hide resolved
I think it is worth adding a small paragraph in the vault's documentation. |
f572a1f
to
d292a97
Compare
@vsevel I've updated the docs to reference DevServices support and configuration and fixed the version inconsistency with You said you are not familiar with DevServices. Who should be merging this? |
@gsmet You reviewed and merged the Redis DevServices change, can you take a look at this as well? |
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 spotted a couple of issues in the doc, could you have another look? Apart from that, it looks good to go, thanks.
79823de
to
3cb70e2
Compare
@vsevel could you check your comments are addressed? If so, it looks good to go. |
Enables DevServices for Vault.
Note that due to my unfamiliarity with how DevServices worked (although it seems mostly clear now) this is implemented essentially a copy of the recent PR #17070 for Redis. If there is complexity that can be removed or the feature can be better organized I'm all ears.