Skip to content
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

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Conversation

mvach
Copy link
Contributor

@mvach mvach commented Dec 4, 2023

No description provided.

Expect(err).ToNot(HaveOccurred())

Expect(*respSnapshots.Snapshots[0].Encrypted).To(BeTrue())
Expect(*respSnapshots.Snapshots[0].KmsKeyId).To(Equal(destinationRegionKmsKeyId))
Copy link
Contributor Author

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.

Copy link
Member

@jpalermo jpalermo left a 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,
Copy link
Contributor Author

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.

Copy link
Contributor

@ramonskie ramonskie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@anshrupani anshrupani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

ramonskie added a commit to cloudfoundry/bosh-stemcells-ci that referenced this pull request Dec 8, 2023
@ramonskie ramonskie merged commit 2160c17 into master Dec 8, 2023
5 checks passed
@ramonskie ramonskie deleted the fix-failing-driver-test branch December 8, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants