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(appsync): ttl property of CachingConfig is not required #17981

Merged
merged 2 commits into from
Dec 14, 2021
Merged

fix(appsync): ttl property of CachingConfig is not required #17981

merged 2 commits into from
Dec 14, 2021

Conversation

otaviomacedo
Copy link
Contributor

@otaviomacedo otaviomacedo commented Dec 13, 2021 •
edited
Loading

According to the current CloudFormaton spec for AWS::AppSync::Resolver CachingConfig, the Ttl property is not required. But if it's not provided, the deploy will fail with the message:

TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

The AppSync team has confirmed that the property is indeed required and they will push a change to the spec. This change is proactively making the property required on the CDK.

Fixes #17925.

BREAKING CHANGE: The CachingConfig#ttl property is now required.

----

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

@gitpod-io
Copy link

gitpod-io bot commented Dec 13, 2021

@otaviomacedo otaviomacedo added the pr-linter/exempt-test The PR linter will not require test changes label Dec 13, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 13, 2021
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Dec 13, 2021
@otaviomacedo otaviomacedo requested a review from a team December 13, 2021 10:20
njlynch
njlynch reviewed Dec 13, 2021
View reviewed changes
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Two thoughts:

  1. Should we wait to make this required in the CDK until the spec change has been merged? Maybe not, but coordinating the two would make some sense.
  2. Either way, this needs to be marked as a BREAKING CHANGE.

@otaviomacedo otaviomacedo added the pr/do-not-merge This PR should not be merged at this time. label Dec 13, 2021
@otaviomacedo
Copy link
Contributor Author

  1. Should we wait to make this required in the CDK until the spec change has been merged? Maybe not, but coordinating the two would make some sense.

I don't mind. We can hold this off until the new spec is out. On the other hand, if this property is left undefined, it will fail one way or the other. With this change, it will fail faster (compile time).

  1. Either way, this needs to be marked as a BREAKING CHANGE.

Done.

@otaviomacedo
Copy link
Contributor Author

Latest update from the AppSync team:

The recommended workaround at this point is to include the TTL field. Our next step is to make this field required and update our docs.

Removing the pr/do-not-merge tag.

@otaviomacedo otaviomacedo removed the pr/do-not-merge This PR should not be merged at this time. label Dec 14, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: e10e993
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 73e5fec into master Dec 14, 2021
@mergify mergify bot deleted the otaviom/appsync-ttl-required branch December 14, 2021 20:39
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

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

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
@otaviomacedo @TikiTDO
fix(appsync): ttl property of CachingConfig is not required (aws#…
e44d527 
…17981)

According to the current CloudFormaton spec for [AWS::AppSync::Resolver CachingConfig][1], the `Ttl` property is not required. But if it's not provided, the deploy will fail with the message:

>  TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

The AppSync team has confirmed that the property is indeed required and they will push a change to the spec. This change is proactively making the property required on the CDK.

Fixes aws#17925.

BREAKING CHANGE: The `CachingConfig#ttl` property is now required. 

[1]: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appsync-resolver-cachingconfig.html#cfn-appsync-resolver-cachingconfig-ttl
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njlynch njlynch njlynch approved these changes

Assignees

@otaviomacedo otaviomacedo

Labels
@aws-cdk/aws-appsync Related to AWS AppSync contribution/core This is a PR that came from AWS. pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(appsync): 2.1.0 breaks deployment
3 participants
@otaviomacedo @aws-cdk-automation @njlynch