-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix failing copy_ami_driver tests #35
Conversation
Expect(err).ToNot(HaveOccurred()) | ||
|
||
Expect(*respSnapshots.Snapshots[0].Encrypted).To(BeTrue()) | ||
Expect(*respSnapshots.Snapshots[0].KmsKeyId).To(Equal(destinationRegionKmsKeyId)) |
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.
Please note that we change the test expectation here.
We never managed to share an AMI with a single region kms key across regions. It's as well documented that this is not working.
After a discussion with @ramonskie it turned out that no one is using encrypted AMIs so far. Therefor we adapted the behaviour in a way that a multi region kms key need to be provided when copying encrypted AMIs across regions. Due to that we check the replicated destination key here.
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.
👍
Currently copy_ami_driver_test depends on the kms_driver_test since the kms_driver_test replicates a key that is used in the copy_ami_driver_test This can result in flakey tests.
@@ -58,7 +58,7 @@ var _ = Describe("KmsDriver", func() { | |||
|
|||
It("replicates a given kms key to another region", func() { | |||
driverConfig := resources.KmsReplicateKeyDriverConfig{ | |||
KmsKeyId: multiregionKmsKeyId, | |||
KmsKeyId: multiRegionKeyReplicationTest, |
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 test triggers the cleanup of the replicated key in line 81. That makes the private AMI tests in the copy_ami_driver_tests flakey. Therefore we have to separate the used multiregion kms keys here.
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.
lgtm
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
No description provided.