Add CDK test infra #706

kukushking · 2021-05-20T09:34:26Z

Issue #: N/A

Description of changes:
Remove template-based CloudFormation integration test infrastructure and add CDK-based one.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jaidisido · 2021-05-20T09:37:53Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: ace4770
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jaidisido · 2021-05-20T23:40:43Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: add0c61
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jaidisido

Many thanks for this, left a few comments. Since you were able to run the entire test suite with this infra, I assume it's all fine

jaidisido · 2021-05-25T15:21:01Z

CONTRIBUTING.md

+
+``cd test_infra``
+
+* Install CDK dependencies:


Should we just clarify that the CDK needs to be correctly set up and configured? Something along the lines of "we assume CDK is correctly bootstrapped and configured in the AWS account..."

jaidisido · 2021-05-25T15:22:26Z

test_infra/.gitignore

@@ -0,0 +1,11 @@
+*.swp


Do we really need 2 .gitignore? A separate repo is not created for the CDK test infra so perhaps we can just use the root .gitignore?

Yep, agree that is better.

jaidisido · 2021-05-25T15:23:37Z

test_infra/README.md

@@ -0,0 +1,62 @@
+


Do we need both this README and the contributing guide?

Nope, will remove.

jaidisido · 2021-05-25T15:26:34Z

test_infra/requirements.txt

@@ -0,0 +1,10 @@
+aws-cdk.core


I am torn on this one:

I suppose it's better to split the aws-cdk requirements from our actual requirements-dev.txt but on the other it's a separate requirements file that we need to manage

I see that no version is specified. I think it makes sense otherwise dependabot would be constantly raising PRs but at the same time latest versions of the CDK libraries are more likely to have issues/bugs...

On 1 - my main reasoning was that, for example, you may want to only run mocked tests and there is no reason for you to install CDK. I feel like most of the users don't go as far as spinning up their own test infrastructure.
On 2 - good catch, the version should definitely be fixed.

1 makes perfect sense. For 2, it will be easier to manage with CDK v2 but for now I am not sure we want to fix the versions as dependabot will go wild haha

jaidisido · 2021-05-25T15:28:49Z

test_infra/scripts/deploy-base.sh

+set -e
+
+pushd ..
+cdk bootstrap


Hmm I am not sure we should be responsible for the bootstrapping of the account? User might have already done it or is using a custom bootstrap template. Perhaps clearly explaining in the contributing guide that the account should be bootstrapped is enough?

The reasoning behind it was that I'd like to simplify the instructions as much as possible. Less instructions to follow - users are less likely to get scared by them. If the account is bootstrapped already, there is no harm in running the command again.

One point to ponder though: there may be issues if it's bootstrapped with a different version - I have seen failures due to that. Then having this script may obscure a bit what exactly is going on, although, the cdk stdout is hard to ignore :) and I think it will tell you the version difference.

Fair enough, we can keep the bootstrap part

jaidisido · 2021-05-25T15:42:28Z

test_infra/test_infra/base_stack.py

@@ -0,0 +1,129 @@
+from aws_cdk import aws_ec2 as ec2


Minor one, can we rename the directory to "stacks" instead of "test_infra"? Just to avoid having two directories named "test_infra"

I was trying to follow the usual naming standards for python packages here e.g. there's the root of the repo, and then python package inside with very often the same name. I'm all right to change it, though.

Happy with convention 👍🏼

jaidisido · 2021-05-25T15:48:10Z

test_infra/test_infra/databases_stack.py

+
+    def _set_db_infra(self) -> None:
+        self.db_username = "test"
+        self.db_password = cdk.CfnParameter(self, "dbpassword", type="String").value_as_string


no echo should be set to true like in the CFN template

cdk.CfnParameter(self, "dbpassword", type="String", no_echo=True)

Link: https://docs.aws.amazon.com/cdk/api/latest/python/aws_cdk.core/CfnParameter.html

This should really be a secret instead, agree about the mask though.

jaidisido · 2021-05-25T15:52:25Z

test_infra/test_infra/databases_stack.py

+            },
+        )
+        cdk.CfnOutput(self, "DatabasesUsername", value=self.db_username)
+        cdk.CfnOutput(self, "DatabasesPassword", value=self.db_password)


Hmm now that I think about it, we are just storing the password in the output section anyways... Perhaps we should move away from that and use an SSM parameter that we can from within our conftest.py?

Yeah, at this point I only ported templates we had to CDK but I agree there are many things to improve. Like, passwords in cleartext make me sweat, too, even if it's just test infra.

jaidisido · 2021-05-25T15:53:37Z

test_infra/test_infra/databases_stack.py

+        self._set_db_infra()
+        self._set_catalog_encryption()
+        self._setup_redshift()
+        self._setup_postgresql()
+        self._setup_mysql()
+        self._setup_sqlserver()


jaidisido · 2021-05-25T15:56:03Z

test_infra/test_infra/databases_stack.py

+        glue.Connection(
+            self,
+            "aws-data-wrangler-mysql-glue-connection-ssl",
+            type=glue.ConnectionType.JDBC,


missing description

kukushking · 2021-05-25T17:03:39Z

tests/test_catalog.py

@@ -150,11 +150,9 @@ def test_catalog(path: str, glue_database: str, glue_table: str, account_id: str


 def test_catalog_get_databases(glue_database):


I feel like this needs a comment @jaidisido - I am removing description assertion here mainly because L2 construct for Glue Database doesn't support it :/

Lol, pretty silly that it does not support it... It's fine, testing against a hard code string was not great anyways

kukushking · 2021-05-26T22:00:46Z

test_infra/test_infra/base_stack.py

+                        actions=["kms:*"],
+                        principals=[iam.AccountRootPrincipal()],
+                        resources=["*"],
+                    )


Hmm, I don't think so - the above statement already allows the root principal (which means effectively any inhabitant of the account) all KMS actions.

kukushking · 2021-05-26T22:01:09Z

test_infra/setup.py

@@ -0,0 +1,32 @@
+import setuptools


Missed that one -- nope, not required.

kukushking · 2021-05-26T22:03:24Z

test_infra/test_infra/base_stack.py

+        self.bucket = s3.Bucket(
+            self,
+            id="aws-data-wrangler",
+            versioned=True,


i remember I found this strange, too, but I don't see it now :) I was probably looking somewhere else. Will remove that one.

jaidisido · 2021-06-01T13:55:55Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: 47322d2
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jaidisido · 2021-06-03T12:14:53Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: d99a650
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jaidisido · 2021-06-03T12:28:31Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: 4c250b0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

* Delete CFN template-based test infrastructure * Update docs * Replace db password string with a secret value

jaidisido · 2021-06-04T12:06:18Z

AWS CodeBuild CI Report

CodeBuild project: GitHubCodeBuild8756EF16-sDRE8Pq0duHT
Commit ID: 252a6cd
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

kukushking requested a review from jaidisido May 20, 2021 09:44

kukushking force-pushed the cdk_test_infra branch from ace4770 to add0c61 Compare May 20, 2021 23:28

jaidisido reviewed May 25, 2021

View reviewed changes

jaidisido assigned kukushking May 25, 2021

jaidisido added the enhancement New feature or request label May 25, 2021

kukushking commented May 26, 2021

View reviewed changes

kukushking force-pushed the cdk_test_infra branch from 7fbc509 to 47322d2 Compare June 1, 2021 13:20

aws deleted a comment from jaidisido Jun 1, 2021

* Add CDK test infrastructure

252a6cd

* Delete CFN template-based test infrastructure * Update docs * Replace db password string with a secret value

kukushking force-pushed the cdk_test_infra branch from 4c250b0 to 252a6cd Compare June 4, 2021 11:54

jaidisido merged commit 89ab547 into aws:main Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CDK test infra #706

Add CDK test infra #706

kukushking commented May 20, 2021

jaidisido commented May 20, 2021

jaidisido commented May 20, 2021

jaidisido left a comment

jaidisido May 25, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 27, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 27, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 27, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 25, 2021

jaidisido May 25, 2021

kukushking May 25, 2021

jaidisido May 27, 2021

kukushking May 26, 2021

kukushking May 26, 2021

kukushking May 26, 2021

jaidisido commented Jun 1, 2021

jaidisido commented Jun 3, 2021

jaidisido commented Jun 3, 2021

jaidisido commented Jun 4, 2021

		@@ -150,11 +150,9 @@ def test_catalog(path: str, glue_database: str, glue_table: str, account_id: str


		def test_catalog_get_databases(glue_database):

Add CDK test infra #706

Add CDK test infra #706

Conversation

kukushking commented May 20, 2021

jaidisido commented May 20, 2021

AWS CodeBuild CI Report

jaidisido commented May 20, 2021

AWS CodeBuild CI Report

jaidisido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaidisido commented Jun 1, 2021

AWS CodeBuild CI Report

jaidisido commented Jun 3, 2021

AWS CodeBuild CI Report

jaidisido commented Jun 3, 2021

AWS CodeBuild CI Report

jaidisido commented Jun 4, 2021

AWS CodeBuild CI Report