-
Notifications
You must be signed in to change notification settings - Fork 2k
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
test: download Vault binaries for e2e test #6938
Conversation
Modernize Vault integration/e2e test a bit: - Download from releases.hashicorp.com instead of using a hardcoded list - Remove old unused make target e2e-test - Use NOMAD_E2E env var instead of -integration flag - Add a README On my machine with ~250 Mbps internet it takes ~400s to download all Vault binaries.
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.
👍 Code changes LGTM, just a couple docs/config questions.
e2e/vault/README.md
Outdated
Run with: | ||
|
||
``` | ||
NOMAD_E2E=1 go test |
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'd update this to specifically call out running this test, i.e. NOMAD_E2E=1 go test -run TestVaultCompatibility
If we implement this in the e2e Framework, we could mark it as a “slow” test when we running the tests locally against, say, AWS clusters. but until the stuff I’m working on for provisioning updates lands (tomorrowish?) those constraints don’t really seem to work completely so probably better to keep it as-is.
g.Go(func() error { | ||
return h.get(version) | ||
sema <- 1 |
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.
[Aside]: A pattern I have seen used a lot is to launch the number of workers you want and have them loop over a channel. And then you can have a go routine pushing into the channel.
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 think that works great when the number of concurrent tasks (downloads in this case) is large, unknown, or unbounded, but when the number of tasks is small this route is less loops and nesting.
I think this is good to merge now, @schmichael ? |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Modernize Vault integration/e2e test a bit:
On my machine with ~250 Mbps internet it takes ~400s to download all
Vault binaries.