-
Notifications
You must be signed in to change notification settings - Fork 466
Conversation
5f16334
to
9822e24
Compare
Please review #87 before this one and I will rebase appropriately |
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.
New tests look good! Just minor comments and merge away.
|
||
func TestVaultClusterEnterpriseWithAmazonLinuxAmi(t *testing.T) { | ||
t.Parallel() | ||
runVaultEnterpriseClusterTest(t, "amazon-linux-ami", "ec2-user", os.Getenv("VAULT_AMI_TEMPLATE_VAR_DOWNLOAD_URL")) |
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.
The use of an env var here won't be obvious to future maintainers. Consider add a method that:
- Explicitly checks if the env var is set
- Reports an error that says "Env var xxx not found. Please set it to a Vault Enterprise download URL to run this test."
- Add a comment to the method explaining that Vault Enterprise URLs should be kept secret, so we use an env var to avoid putting it directly in our code or logs.
test/vault_helpers.go
Outdated
// 3. Deploy that AMI using the example Terraform code | ||
// 4. SSH to a Vault node and initialize the Vault cluster | ||
// 5. SSH to each Vault node and unseal it | ||
// 5. SSH to a Vault node and make sure you can communicate with the nodes via Consul-managed DNS |
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.
5
is repeated twice (if you copy/pasted this comment block, might be a bug in the original!). Also, add another step here: check enterprise version of Vault got installed correctly.
5a29519
to
28212b8
Compare
@infosecgithub