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

[CTX-601] chore: fix various AWS acceptance tests #1662

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Conversation

craigfurman
Copy link
Contributor

@craigfurman craigfurman commented May 31, 2023

chore: fix various AWS acceptance tests

chore: re-enable nightly acceptance tests

This reverts commit 9907b1c.

chore: manual acceptance test job

In order to trigger acceptance tests for certain branches pre-merge, and
without having to wait for a nightly build, you can click "trigger
pipeline" in CircleCI and enter ACC_TESTS=1 as a parameter.

chore: remove references to non-Snyk domains in acc test fixtures


Attempt to fix failing AWS acceptance tests from https://app.circleci.com/pipelines/github/snyk/driftctl/5206/workflows/2425c0ab-f8a5-4594-8605-0e532e55ca88/jobs/13441/tests. Unless otherwise stated, I got a passing test against my sandbox account.

  • TestAcc_Aws_ApiGatewayV2Authorizer, TestAcc_Aws_ApiGatewayAuthorizer: the nodejs 12 lambda runtime has been removed. Using a later one necessitated bumping the provider version. I bumped terraform too.
  • TestAcc_Aws_LambdaEventSourceMapping: similar to above, bumped the lambda runtime, but to a lower version to avoid bumping the provider to a recent version. Driftctl is currently incompatible with new provider versions for this resource due to spurious attr-level drift on the destination_config block.
  • TestAcc_Aws_CloudfrontDistribution, TestAcc_Aws_RDSCluster, TestAcc_Aws_RDSClusterInstance: deleted stray resources from previous failed acc test cleanups
  • TestAcc_Aws_VPC, TestAcc_Aws_NetworkAcl_NonDeep, TestAcc_Aws_NetworkAcl: deleted stray resources. I didn't actually run these tests, though.
  • TestAcc_Aws_S3Bucket_BucketInUsEast1: the most complicated one. The acl and policy attributes on a bucket are deprecated, and the actual non-deprecated policy failed to apply (HTTP 403 from AWS). I have no idea why. These attributes were added in Refacto aws_s3_bucket #471, and it's unclear from that PR why that was important to do. I removed all but one bucket from the test fixture, and removed all policy attachments to get this applying successfully at all. I would hope the relevant bucket policy attachment middlewares catch regressions here, but there's a chance this PR just encodes an already-occurred regression.

There are lots of formatting artifacts from my editor automatically terraform-fmting files, sorry!

@craigfurman craigfurman requested a review from a team as a code owner May 31, 2023 16:25
@craigfurman craigfurman requested review from karniwl and removed request for a team May 31, 2023 16:25
In order to trigger acceptance tests for certain branches pre-merge, and
without having to wait for a nightly build, you can click "trigger
pipeline" in CircleCI and enter `ACC_TESTS=1` as a parameter.
@craigfurman
Copy link
Contributor Author

Triggered manual acceptance tests using the CI capability introduced in the 3rd commit here to see if this is an improvement 🤞 https://app.circleci.com/pipelines/github/snyk/driftctl/5228/workflows/0f811d60-10a5-431e-a7fd-8887d21d4bd0

Copy link
Contributor

@karniwl karniwl left a comment

Choose a reason for hiding this comment

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

left one small comment, but other than that, am I missing something or did we want to update the email to replace old cloudskiff ones?

function_name = "event-source-mapping-test-lambda-${local.timestamp}"
role = aws_iam_role.iam_for_lambda.arn
handler = "exports.test"
runtime = "nodejs14.x"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason this one is node 14 and line 89 at pkg/resource/aws/testdata/acc/aws_apigatewayv2_authorizer/terraform.tf is node 18? can't spot the difference and the original one are both the same version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See description 🙂

@craigfurman
Copy link
Contributor Author

@karniwl good call, I've pushed a commit to change those GCP fixtures just now. I haven't run those tests yet, but GCP acc tests are already broken - and are next on the list to fix! This should alleviate concerns around those IAM fixtures in the mean time.

@craigfurman craigfurman requested a review from karniwl June 1, 2023 09:49
Copy link
Contributor

@karniwl karniwl left a comment

Choose a reason for hiding this comment

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

🧪 🧹

@craigfurman craigfurman merged commit fa65e27 into main Jun 1, 2023
@craigfurman craigfurman deleted the acctest-fixes branch June 1, 2023 09:53
craigfurman added a commit that referenced this pull request Jun 1, 2023
One test fixture had a duplicate provider block, rendering it invalid.
Others contained references to users that are actually groups, a recent
regression in #1662.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants