Add tests based on vault binary #20224
Conversation
…e go test case, take 2
…it, so instead skip each step. Yuck.
| // Prevent server startup if migration is active | ||
| // TODO: Use OpenTelemetry to integrate this into Diagnose | ||
| if c.storageMigrationActive(backend) { | ||
| return 1 |
There was a problem hiding this comment.
Will this not result in the process quietly failing with no explanation? Should we have a c.UI.Warn() here to explain why the process is exiting?
There was a problem hiding this comment.
This isn't new to my PR.
command/server.go
Outdated
| } | ||
| } | ||
|
|
||
| if c.flagDevClusterJson != "" { |
There was a problem hiding this comment.
This seems redundant to me. The enableThreeNodeDevCluster function already has this check and writes out the file is the flag is set.
There was a problem hiding this comment.
Good point. We still want to support this option for the single node (-dev) case, but I've changed it so that we don't do this twice in three-node mode (fa42073).
sdk/helper/testcluster/exec.go
Outdated
| } | ||
|
|
||
| func (e *execDevClusterNode) TLSConfig() *tls.Config { | ||
| return e.Client.CloneConfig().TLSConfig() |
There was a problem hiding this comment.
❓ I noticed that TLSConfig() returns a clone of the tls.Config pointed struct. Is the reason that the api.Client's config is also cloned (i.e. e.Client.CloneConfig()), that it can take advantage of the ReadLock for the overall client config?
There was a problem hiding this comment.
No, it's because CloneConfig is the only way to get access to the config field.
… tests can run in parallel (though user must ensure different test clusters get different addresses.)
ncabatoff
changed the title
| DisablePerformanceStandby: true, | ||
| }, | ||
| }, | ||
| BinaryPath: binary, |
There was a problem hiding this comment.
What's your thoughts about an auto value? It'd be cool to just set this globally and have it automatically find it from the bin/vault of the repo that its running tests from.
Then I can set it from the base shell profile and have it magically work across OSS/ENT/... -- worst case the binary is out of date if I haven't rebuilt it before running tests, but at least they'll run by default more often (and CI will obviously pick up if there's an unexpected local test pass due to mismatched binary version).
This lets the empty string also still mean "don't run these tests".
Edited to add, it looks like if an empty BinaryPath gets executed, it will reset the binary to vault, which I think will be $PATH expanded, and so might be right a build was last executed in this repo, but wouldn't be right if you've built each repo (OSS+ENT) and switched between the two without rebuilding again (as the version under ~/go/bin would be static without rebuild or manual update). But the empty string is still used for Skip, so that default vault value would be unused.
There was a problem hiding this comment.
Note that typically the bin/vault isn't going to be the right GOOS (and possibly not the right GOARCH) for use in docker.
I'm not averse to us taking steps to run these tests automatically locally, but I'm inclined to save that for a future iteration. Ideally I'd want it to actually build the binary if missing/stale, though maybe that's something we could instead delegate to the IDE.
There was a problem hiding this comment.
Ahh I see. Hmm... seems like we'd need to add $GOOS/$GOARCH into the bin path somehow, but that does end up being more work in our build scripts. A future problem then!
This PR includes support for tests using vault binary in -dev or -dev-three-node modes. It also makes -dev-three-node enable unauthenticated pprof and metrics requests and adds support for writing a cluster.json file in -dev and -dev-three-node modes.