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(cfnspec): cloudformation spec v51.0.0 #17955

Merged
merged 7 commits into from
Dec 13, 2021
Merged

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Dec 10, 2021

Closes #17943.


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 10, 2021

@robertd robertd marked this pull request as draft December 10, 2021 17:17
@robertd
Copy link
Contributor Author

robertd commented Dec 10, 2021

Just checking to see why is #17943 failing to build properly. I cannot replicate this on my Mac.

edit: I had to run git clean -fqdx . first.

@robertd
Copy link
Contributor Author

robertd commented Dec 10, 2021

I do see that docs are completely missing Type property...
image

@robertd
Copy link
Contributor Author

robertd commented Dec 10, 2021

Actually... examples are missing too...
image

@robertd
Copy link
Contributor Author

robertd commented Dec 10, 2021

Hmm... AWS Lex docs are nowhere to be found in https://github.com/awsdocs/aws-cloudformation-user-guide/ repo, but they are present on the AWS User Guide page: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/AWS_Lex.html

@skinny85
Copy link
Contributor

Thanks for your work on this @robertd! Two things:

  1. Let's just remove the incorrect Property types, instead of guessing they are a string.
  2. Can you check whether you can now remove the patches introduced in the previous PR (feat(cfnspec): cloudformation spec v50.0.0 #17844)?

Thanks,
Adam

@robertd
Copy link
Contributor Author

robertd commented Dec 11, 2021

  1. Let's just remove the incorrect Property types, instead of guessing they are a string.

Removing the Type property from AWS::Lex::BotAlias.TextLogDestination.Properties.CloudWatch still throws an error. It looks like Type (or PrimitiveType) is a required param. Should I instead remove AWS::Lex::BotAlias.TextLogDestination section altogether?

image

  1. Can you check whether you can now remove the patches introduced in the previous PR (feat(cfnspec): cloudformation spec v50.0.0 #17844)?

Removed AWS::Evidently patches introduced in #17844. Build and test passed on my machine. Pushed fd37bf8.

@skinny85
Copy link
Contributor

skinny85 commented Dec 11, 2021

  1. Let's just remove the incorrect Property types, instead of guessing they are a string.

Removing the Type property from AWS::Lex::BotAlias.TextLogDestination.Properties.CloudWatch still throws an error. It looks like Type (or PrimitiveType) is a required param. Should I instead remove AWS::Lex::BotAlias.TextLogDestination section altogether?

image

Sure, that should work.

Or remove AWS::Lex::BotAlias.TextLogDestination.Properties.CloudWatch.

@robertd robertd marked this pull request as ready for review December 11, 2021 22:27
@robertd
Copy link
Contributor Author

robertd commented Dec 11, 2021

@skinny85 Done. Build is passing now. 👍

  1. Should we make a note (GH Issue) to revisit this in the future cfnspec bumps?
  2. I see bunch of other patches that are probably not needed anymore. Should we clean these up at some point?
  3. Can you point me to the "official cfnspec patching how-to documentation" (if it exists). I'd like to learn more about it :) (i.e. resource patching vs property patching, available operations/commands, etc).

@mergify
Copy link
Contributor

mergify bot commented Dec 13, 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).

@skinny85
Copy link
Contributor

@skinny85 Done. Build is passing now. 👍

Thanks for all your work on these CFN spec updates @robertd!

  1. Should we make a note (GH Issue) to revisit this in the future cfnspec bumps?

That would be great!

  1. I see bunch of other patches that are probably not needed anymore. Should we clean these up at some point?

We absolutely should! I don't expect you to handle that though, I think you've done enough 🙂.

  1. Can you point me to the "official cfnspec patching how-to documentation" (if it exists). I'd like to learn more about it :) (i.e. resource patching vs property patching, available operations/commands, etc).

There is no official documentation for this, as not many tools use the CFN spec, and even less of them patch it 🙂. The mechanism we use I'm fairly sure we "borrowed" (😉) from the cfnlint tool.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 6d4d112
  • 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 c6b7a49 into aws:master Dec 13, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 13, 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
Closes aws#17943.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@dontirun
Copy link
Contributor

It looks like like v60.0.0 of the spec supports this property properly

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants