Skip to content

feat(cfnspec): cloudformation spec v51.0.0#17955

Merged
mergify[bot] merged 7 commits intoaws:masterfrom
robertd:update-cfnspec
Dec 13, 2021
Merged

feat(cfnspec): cloudformation spec v51.0.0#17955
mergify[bot] merged 7 commits intoaws:masterfrom
robertd:update-cfnspec

Conversation

@robertd
Copy link
Copy Markdown
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
Copy Markdown

gitpod-io bot commented Dec 10, 2021

@robertd robertd marked this pull request as draft December 10, 2021 17:17
@robertd
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

robertd commented Dec 10, 2021

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

@robertd
Copy link
Copy Markdown
Contributor Author

robertd commented Dec 10, 2021

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

@robertd
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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