Skip to content

Conversation

@fuelvin1
Copy link
Contributor

Description of changes

  • Adds new methods for alternate flow using the Aurora serverless V2 Postgres clusters as part of setup and teardown.
  • Uses Data API to provision test database and tables.
CDK / CloudFormation Parameters Changed

Issue #, if available

Description of how you validated changes

Update sql-models-2 test to use postgres and verified it passes.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

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

@fuelvin1 fuelvin1 marked this pull request as ready for review July 31, 2024 22:48
@fuelvin1 fuelvin1 requested review from a team as code owners July 31, 2024 22:48
@fuelvin1 fuelvin1 marked this pull request as draft July 31, 2024 22:49
@fuelvin1 fuelvin1 force-pushed the tests-use-data-api branch from f162a71 to 73e09f1 Compare August 1, 2024 23:08
@fuelvin1 fuelvin1 marked this pull request as ready for review August 2, 2024 17:28
const createDBInput: ExecuteStatementCommandInput = {
resourceArn: dbCluster.clusterArn,
secretArn,
sql: `create database ${config.dbname};`,
Copy link
Member

Choose a reason for hiding this comment

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

Rework this to include dbname as a parameter, to avoid potential SQL injection if dbname is ever compromised. (Plus, it's a good habit to get into to not interpolate into SQL if you can avoid it). In this case, the risk is minimal since we generate the dbname, but if in future we decided "hey, let's let dbname be specified by an environment variable", then we'd be open to executing SQL with untrusted input.

phani-srikar
phani-srikar previously approved these changes Aug 5, 2024
},
};

const command = new CreateDBClusterCommand(params);
Copy link
Member

Choose a reason for hiding this comment

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

nit: command is pretty generic, especially since you're issuing multiple commands in the body of this function

Suggested change
const command = new CreateDBClusterCommand(params);
const createClusterCommand = new CreateDBClusterCommand(params);

const instanceCommand = new CreateDBInstanceCommand(instanceParams);

try {
const rdsResponse = await rdsClient.send(command);
Copy link
Member

Choose a reason for hiding this comment

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

As above, rdsResponse is pretty generic. Consider something that more explicitly defines the purpose of the variable

Suggested change
const rdsResponse = await rdsClient.send(command);
const createClusterResponse = await rdsClient.send(createClusterCommand);

database: dbCluster.dbName,
};

const command = new ExecuteStatementCommand(createDBInput);
Copy link
Member

Choose a reason for hiding this comment

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

nit: as elsewhere, the name is pretty generic

Suggested change
const command = new ExecuteStatementCommand(createDBInput);
const createDbCommand = new ExecuteStatementCommand(createDBInput);

// create the test tables in the test database
queries?.map(async (query) => {
try {
const createTableInput: ExecuteStatementCommandInput = {
Copy link
Member

Choose a reason for hiding this comment

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

very nit: you don't actually know that the incoming queries are for creating tables -- some may be altering tables, creating indexes, or even updating tables with new records. :) In this case, a more generic name is appropriate.

Suggested change
const createTableInput: ExecuteStatementCommandInput = {
const executeStatementInput: ExecuteStatementCommandInput = {

Comment on lines 350 to 351
const createTableResponse = await client.send(new ExecuteStatementCommand(createTableInput));
console.log('Create table response: ' + JSON.stringify(createTableResponse));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const createTableResponse = await client.send(new ExecuteStatementCommand(createTableInput));
console.log('Create table response: ' + JSON.stringify(createTableResponse));
const executeStatementResponse = await client.send(new ExecuteStatementCommand(createTableInput));
console.log('executeStatementResponse: ' + JSON.stringify(executeStatementResponse));

Comment on lines +173 to +174
MinCapacity: 4,
MaxCapacity: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is only a test resource, can we lower these values?

@phani-srikar phani-srikar merged commit 5084979 into main Aug 6, 2024
@phani-srikar phani-srikar deleted the tests-use-data-api branch August 6, 2024 21:41
@sundersc sundersc restored the tests-use-data-api branch August 9, 2024 22:33
@palpatim palpatim deleted the tests-use-data-api branch November 25, 2024 22:13
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.

4 participants