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

secrets/vault: run tests against a vault docker container #1983

Merged
merged 10 commits into from
May 6, 2019

Conversation

shantuo
Copy link
Contributor

@shantuo shantuo commented May 2, 2019

Fixes #1958

@shantuo shantuo requested a review from eliben May 2, 2019 19:13
@googlebot googlebot added the cla: yes Google CLA has been signed! label May 2, 2019
@eliben
Copy link
Member

eliben commented May 2, 2019

Also related to #886

Copy link
Member

@eliben eliben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for fixing this quickly! I'd double check with @zombiezen for the docker-y stuff

@eliben eliben requested a review from zombiezen May 2, 2019 20:10
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #1983 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
- Coverage   77.63%   77.49%   -0.15%     
==========================================
  Files          82       82              
  Lines        9566     9566              
==========================================
- Hits         7427     7413      -14     
- Misses       1607     1621      +14     
  Partials      532      532
Impacted Files Coverage Δ
health/sqlhealth/sqlhealth.go 61.9% <0%> (-19.05%) ⬇️
internal/retry/retry.go 88.23% <0%> (-11.77%) ⬇️
secrets/vault/vault.go 77.04% <0%> (-6.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 323a32a...e61fd24. Read the comment docs.

Copy link
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Less dependencies, yay! One area to think about, though:

"gocloud.dev/secrets"
"gocloud.dev/secrets/driver"
"gocloud.dev/secrets/drivertest"
)

// To run these tests against a real Vault server, first run ./localvault.sh.
// Then wait a few seconds for the server to be ready.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can health check it for readiness? Or perhaps dial-looping for some period of time? This seems like a source of flakiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default client has 2 max retries and 60-second timeout. I changed it to 3 max retries with 3-second timeout for local testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I do wonder whether start_local_deps.sh or localvault.sh should wait for a healthy response before continuing with the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about doing some checks on the server health and retries. But I think it is hard to know whether we should restart the container or wait longer for the health probe. It would become as flaky. That's why I did more retries when dialing the test server and skip the test after a few attempts. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm saying is: the test is using presence of server to determine whether to run the test, so that should fail fast. The localvault.sh script should block on health checks before coming up. This is likely a problem with all current local dep scripts, so it would be fine to file an issue and handle it separately. In the meantime, I think these tests should fail fast.

Copy link
Contributor Author

@shantuo shantuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

"gocloud.dev/secrets"
"gocloud.dev/secrets/driver"
"gocloud.dev/secrets/drivertest"
)

// To run these tests against a real Vault server, first run ./localvault.sh.
// Then wait a few seconds for the server to be ready.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default client has 2 max retries and 60-second timeout. I changed it to 3 max retries with 3-second timeout for local testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secrets/vault: stop depending on Vault root module in test
5 participants