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

feat(rds): add clusterArn property to DatabaseCluster #29289

Closed
wants to merge 1 commit into from

Conversation

blimmer
Copy link
Contributor

@blimmer blimmer commented Feb 28, 2024

Reason for this change

It wasn't clear to me how to get the clusterArn from DatabaseCluster. This PR adds that property.

Description of changes

Add clusterArn to DatabaseCluster, just like ServerlessCluster exposes.

Description of how you validated changes

Added unit test similar to ServerlessCluster.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team February 28, 2024 01:09
@github-actions github-actions bot added p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Feb 28, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

Comment on lines +688 to +699
/**
* The ARN of the cluster
*/
public get clusterArn(): string {
return Stack.of(this).formatArn({
service: 'rds',
resource: 'cluster',
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
resourceName: this.clusterIdentifier,
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like ServerlessCluster

/**
* The ARN of the cluster
*/
public get clusterArn(): string {
return Stack.of(this).formatArn({
service: 'rds',
resource: 'cluster',
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
resourceName: this.clusterIdentifier,
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@blimmer Is this how CDK normally handles this? I ask because the output becomes a Join instead of doing a GetAtt on the resource. Trying to wrap my head around why we should do this over the GetAtt path. I tried looking at the Lambda Function L2, since there is a readonly property for the Arn.

Honestly not sure if it matters (likely not) but trying to learn different parts of this as I go. To me seems like we have two patterns going on, the one you have here and the readonly property for the Arn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure - I just followed the pattern in the linked code. One of the core contributors might be able to tell us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked the team directly. I did find SQS which does this differently. https://github.com/aws/aws-cdk/blob/v2.130.0/packages/aws-cdk-lib/aws-sqs/lib/queue.ts#L437-L440

const queue = new sqs.Queue(this, 'CdkExampleQueue', {
      visibilityTimeout: Duration.seconds(300)
    });

new CfnOutput(this, "arn", {
      exportName: "arn",
      value: queue.queueArn,
    })

CFN Synth

Outputs:
  arn:
    Value:
      Fn::GetAtt:
        - CdkExampleQueue7618E31B
        - Arn
    Export:
      Name: arn

Which fits my mental model better. I am not sure if there is a big difference or a reason it was done this way in RDS. Maybe one is a "legacy" way of doing things.

Generally, this looks fine. I just want to get some deeper clarity on what is typical.

Copy link
Contributor

@colifran colifran Mar 1, 2024

Choose a reason for hiding this comment

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

@jfuss Sorry to jump in - I was looking at something similar recently. This is a very typical pattern that is used in the codebase here are just a few examples:

return Stack.of(this).formatArn({

return cdk.Stack.of(this).formatArn({

return Stack.of(this).formatArn({

Also, this is just a "getter" for the clusterArn so I don't think we want to be producing any outputs in the synthesized template. Instead, this just allows clusterArn to be exposed for use throughout the rest of their app.

@@ -4052,6 +4052,34 @@ describe('cluster', () => {
],
});
});

test('check that clusterArn property works', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test('check that clusterArn property works', () => {
// GIVEN
const stack = testStack();
const vpc = ec2.Vpc.fromLookup(stack, 'VPC', { isDefault: true });
const cluster = new ServerlessCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA_MYSQL,
vpc,
});
const exportName = 'DbCluterArn';
// WHEN
new cdk.CfnOutput(stack, exportName, {
exportName,
value: cluster.clusterArn,
});
// THEN
expect(stack.resolve(cluster.clusterArn)).toEqual({
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':rds:us-test-1:12345:cluster:',
{ Ref: 'DatabaseB269D8BB' },
]],
});
});

@blimmer blimmer changed the title feat(aws-rds): add clusterArn property to DatabaseCluster feat(rds): add clusterArn property to DatabaseCluster Feb 28, 2024
@blimmer
Copy link
Contributor Author

blimmer commented Feb 28, 2024

Exemption request. The JSDoc should be sufficient and I added a unit test.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 28, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: fc2dc36
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 28, 2024
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@jfuss jfuss self-assigned this Feb 29, 2024
@go-to-k
Copy link
Contributor

go-to-k commented Mar 1, 2024

Is there a duplication with this PR?

#29133

@jfuss
Copy link
Contributor

jfuss commented Mar 1, 2024

@blimmer Thanks for spending the time. I missed there was another PR already for this and was merged yesterday: #29133.

Closing this as it is a duplicate

@jfuss jfuss closed this Mar 1, 2024
@blimmer blimmer deleted the add-cluster-arn-to-db-cluster branch March 1, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants