Updated Vault docs, uplifted Vault image version #3685
Conversation
- removed VaultClientTest and moved its contest to VaultContainerTest - uplifted Vault image version - added getVaultAddress method for usage convenience
RamazanYapparov
requested review from
bsideup,
kiview and
rnorth
as code owners
|
I'm not a vault user but looks good at a first glance. |
| } | ||
|
|
||
| @Test | ||
| public void readFirstSecretPathOverHttpApi() throws InterruptedException { | ||
| // readWithHttpApi { |
There was a problem hiding this comment.
This comment isn't really necessary - we could have the codeinclude tag in the docs just say: inside_block:readFirstSecretPathOverHttpApi.
This way, we avoid having the method signature public void readFirstSecretPathOverHttpApi() appear in the docs, which is a bit distracting.
| } | ||
|
|
||
| @Test | ||
| public void readFirstSecretPathOverHttpApi() throws InterruptedException { | ||
| // readWithHttpApi { | ||
| public void readFirstSecretPathOverHttpApi() { | ||
| given(). |
There was a problem hiding this comment.
I know the given/when/then style test was here originally, but would mind refactoring these test methods to use plain style (e.g. readFirstSecretPathOverClientLibrary)?
It feels really strange to see given/when/then appearing in the documentation:
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
There was a problem hiding this comment.
I like the docs improvements and the new getVaultAddress. @RamazanYapparov is there a chance to rebase the branch, resolve the conflicts (for example, we already use assertj) and make the changes requested?
modules/vault/src/main/java/org/testcontainers/vault/VaultContainer.java
Outdated
Show resolved
Hide resolved
|
@RamazanYapparov thanks for this PR! few comments were addressed but now this is merged in |
VaultClientTestclass and moved its test toVaultContainerTestgetVaultAddressmethod for usage convenienceThis PR is also related to #1158 issue