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.

Sign up for GitHub

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

Jump to bottom

fix(aws_route53): cannot use CfnParameter.valueAsNumber for L2 RecordSet weight #31823

Merged
merged 8 commits into from
Oct 31, 2024
Merged

fix(aws_route53): cannot use CfnParameter.valueAsNumber for L2 RecordSet weight #31823

merged 8 commits into from
Oct 31, 2024

Conversation

nicholaschiasson
Copy link
Contributor

Issue # (if applicable)

Closes #31810.

Reason for this change

Could not use CfnParameter.valueAsNumber for L2 RecordSet weight.

Description of changes

Adding validation of weight property as a potential Token in RecordSet constructor.

Description of how you validated changes

Added unit and integration test.

Checklist

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

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 21, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team October 21, 2024 00:29
@github-actions github-actions bot added bug This issue is a bug. p2 labels Oct 21, 2024
@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch 3 times, most recently from 8f6bc18 to c5d2522 Compare October 22, 2024 23:10
GavinZZ
GavinZZ previously approved these changes Oct 22, 2024
View reviewed changes
@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from c5d2522 to cf28e09 Compare October 23, 2024 06:59
@mergify mergify bot dismissed GavinZZ’s stale review October 23, 2024 07:00

Pull request has been modified.

@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from cf28e09 to 616536e Compare October 23, 2024 16:16
@GavinZZ
Copy link
Contributor

GavinZZ commented Oct 23, 2024

@nicholaschiasson The integ test aws-route53/test/integ.record-weight-from-cfnparameter failed, would you be able to fix that and I'm happy to approve again to get this merged in.

@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from 529a297 to bbdf81b Compare October 24, 2024 23:15
@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 Oct 25, 2024
@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from 520e8c7 to 18b423d Compare October 25, 2024 09:33
@nicholaschiasson
Copy link
Contributor Author

@GavinZZ thanks! The integ test is now fixed.

GavinZZ
GavinZZ previously approved these changes Oct 25, 2024
View reviewed changes
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 25, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 25, 2024
@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from 4c068da to 0b850c4 Compare October 27, 2024 13:40
@mergify mergify bot dismissed GavinZZ’s stale review October 27, 2024 13:41

Pull request has been modified.

@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 Oct 27, 2024
@nicholaschiasson
Copy link
Contributor Author

Hey @GavinZZ , sorry, I had misunderstood the first time around why the integration test was unstable, but I am confident I have solved it now. The issue was related to how the set identifier was derived if one was not provided with constructor options.

I realized that when generating a set ID, the method restricts the length of the ID to 64 characters. Since token.valueAsNumber produces a very long and non-deterministic-length (stringified) number, the test could now fail randomly. (Eg. -1.8881545897089744e+289)

To solve this, I modified the configureSetIdentifier method to check if the weight property is a token, and in that case, essentially mock the weight value as a three character string, since we can't know the parameter value at synthesis time, but we know that the maximum value allowed for weight is 255. After generating the identifier, I inject the Token as string instead of as number, just so it may be more helpful for a developer maybe for debugging sake (Eg. ${Token[TOKEN.1037]}). Regardless of if it is asNumber or asString, this will be substituted during synthesis. During CDK runtime however, maybe it could be helpful.

Let me know if this is a satisfactory solution. At the very least, the integ test should now be stable.

@github-actions github-actions bot mentioned this pull request Oct 28, 2024
Closed
@nicholaschiasson nicholaschiasson force-pushed the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch from 85dcf0b to 78364da Compare October 28, 2024 16:06
GavinZZ
GavinZZ previously approved these changes Oct 28, 2024
View reviewed changes
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 28, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@GavinZZ GavinZZ dismissed their stale review October 28, 2024 17:05

Dismissing as I notice that there's code change. Will review the new code.

GavinZZ
GavinZZ requested changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

One minor comment for a slight improvement, and I'm happy to approve again.

packages/aws-cdk-lib/aws-route53/lib/record-set.ts Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 28, 2024
GavinZZ
GavinZZ previously approved these changes Oct 29, 2024
View reviewed changes
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 29, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@nicholaschiasson
Copy link
Contributor Author

Hi @GavinZZ , it seems like something is blocking the merge. Do you have any detail about that?

@mergify mergify bot dismissed GavinZZ’s stale review October 31, 2024 17:22

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 31, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 14561ac into aws:main Oct 31, 2024
13 checks passed
@github-actions GitHub Actions
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2024
@nicholaschiasson nicholaschiasson deleted the nicholaschiasson/resolve-recordset-weight-from-cfnparameter branch November 1, 2024 08:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GavinZZ GavinZZ GavinZZ approved these changes

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws_route53: cannot use CfnParameter.valueAsNumber for L2 RecordSet weight
3 participants
@nicholaschiasson @GavinZZ @aws-cdk-automation