-
Notifications
You must be signed in to change notification settings - Fork 13
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
Alter BuildTargets and Setup function signatures to accept a struct with more info #138
Conversation
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 start! I do think there are some changes we may want to consider which I commented on.
a4942cf
to
02bef3b
Compare
Co-authored-by: Luis (LT) Carbonell <lt.carbonell@hashicorp.com>
Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com>
Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com>
2ab227a
to
cb45db9
Compare
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.
One question but looks good to me!
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 good!
Initially I wanted to simply send the VaultBenchmarkCoreConfig struct in this BuildTargets function. But the
config
package imports thebenchmarktests
package, so I couldn't importconfig
intobenchmarktests
because it would be a circular dependency. Instaed, I created a new struct that lives inbenchmarktests
that represents the "top level" configuration (nothing test specific) calledTopLevelTargetConfig
.This allows us to do more with our setup function. For example, check if the max_exp_jwt of the GCP role is less than the duration of the tests, an error will be returned immediately. (This GCP specific logic will be added in this follow up [PR])(#139).