-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add tests based on vault binary #20224
Conversation
…e go test case, take 2
…it, so instead skip each step. Yuck.
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.
Looks great, can't wait to use the new flags for local testing. I do have some things that I pointed out.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't new to my PR.
command/server.go
Outdated
} | ||
} | ||
|
||
if c.flagDevClusterJson != "" { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
DisablePerformanceStandby: true, | ||
}, | ||
}, | ||
BinaryPath: binary, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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.
Looks like a lot of nice changes; looking forward to #20247 :D
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.