Fix vault module, add new test with java client v2 #1791
Conversation
(cherry picked from commit 8931f86)
adds tests for default constructor sets default image to vault:latest
| } | ||
|
|
||
| public VaultContainer(String dockerImageName) { | ||
| super(dockerImageName); | ||
|
|
||
| setWaitStrategy(Wait.forHttp("/v1/secret/is_alive").forStatusCode(400)); |
There was a problem hiding this comment.
400 seems like an odd status code to be waiting for. I can imagine how this could be perfectly correct, but perhaps this deserves at least a comment?
Does anyone recall the reason for this?
There was a problem hiding this comment.
Wouldn't it make sense for this to be:
setWaitStrategy(Wait.forHttp("/v1/sys/health").forStatusCode(200));
(looking at https://www.vaultproject.io/api/system/health.html)
modules/vault/src/main/java/org/testcontainers/vault/VaultContainer.java
Outdated
Show resolved
Hide resolved
|
@Casz sorry for the late posting of comments. Would you still be willing to push this PR through? |
…ainer.java Co-Authored-By: Richard North <rich.north@gmail.com>
* Move customizations to constructor to allow overrides * Use a single startup attempt, as 1 should be sufficient
|
I took the liberty of updating the PR to reflect my comment - I hope this is OK. @bsideup, @kiview would one of you mind sanity checking my change? Additionally I noticed a couple of little things that are being flagged by the IDE - but not related to this PR at all. I'll raise a separate PR to address those. |
|
All good @rnorth THANKS! sorry for not getting back to you 😭 |
|
No probs @Casz! |
|
LGTM 👍 |
rnorth
changed the title
resolves #1410
Using JSON and removing all the whitespace is one fix.
Perhaps we should add a second method in the that will call CLI using format=JSON and remove the whitespace 😓