-
Notifications
You must be signed in to change notification settings - Fork 697
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
Add CDK test infra #706
Add CDK test infra #706
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
ace4770
to
add0c61
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
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
|
||
``cd test_infra`` | ||
|
||
* Install CDK dependencies: |
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.
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..."
test_infra/.gitignore
Outdated
@@ -0,0 +1,11 @@ | |||
*.swp |
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.
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?
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.
Yep, agree that is better.
test_infra/README.md
Outdated
@@ -0,0 +1,62 @@ | |||
|
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.
Do we need both this README and the contributing guide?
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.
Nope, will remove.
test_infra/requirements.txt
Outdated
@@ -0,0 +1,10 @@ | |||
aws-cdk.core |
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.
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...
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.
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.
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.
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
set -e | ||
|
||
pushd .. | ||
cdk bootstrap |
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.
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?
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 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.
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.
Fair enough, we can keep the bootstrap part
test_infra/test_infra/base_stack.py
Outdated
@@ -0,0 +1,129 @@ | |||
from aws_cdk import aws_ec2 as ec2 |
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.
Minor one, can we rename the directory to "stacks" instead of "test_infra"? Just to avoid having two directories named "test_infra"
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.
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.
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.
Happy with convention 👍🏼
|
||
def _set_db_infra(self) -> None: | ||
self.db_username = "test" | ||
self.db_password = cdk.CfnParameter(self, "dbpassword", type="String").value_as_string |
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.
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
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 should really be a secret instead, agree about the mask though.
}, | ||
) | ||
cdk.CfnOutput(self, "DatabasesUsername", value=self.db_username) | ||
cdk.CfnOutput(self, "DatabasesPassword", value=self.db_password) |
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.
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?
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.
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.
self._set_db_infra() | ||
self._set_catalog_encryption() | ||
self._setup_redshift() | ||
self._setup_postgresql() | ||
self._setup_mysql() | ||
self._setup_sqlserver() |
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.
Nice!
glue.Connection( | ||
self, | ||
"aws-data-wrangler-mysql-glue-connection-ssl", | ||
type=glue.ConnectionType.JDBC, |
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.
missing description
@@ -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): |
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.
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 :/
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.
Lol, pretty silly that it does not support it... It's fine, testing against a hard code string was not great anyways
test_infra/test_infra/base_stack.py
Outdated
actions=["kms:*"], | ||
principals=[iam.AccountRootPrincipal()], | ||
resources=["*"], | ||
) |
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.
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.
test_infra/setup.py
Outdated
@@ -0,0 +1,32 @@ | |||
import setuptools |
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.
Missed that one -- nope, not required.
test_infra/test_infra/base_stack.py
Outdated
self.bucket = s3.Bucket( | ||
self, | ||
id="aws-data-wrangler", | ||
versioned=True, |
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.
i remember I found this strange, too, but I don't see it now :) I was probably looking somewhere else. Will remove that one.
7fbc509
to
47322d2
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
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
4c250b0
to
252a6cd
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.