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

chore(ir): add specific error message when an invalid resource property is used #594

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

colifran
Copy link
Contributor

@colifran colifran commented Mar 5, 2024

In the event that an invalid property is specified for a specific resource type the value_type will be None causing the other arm of the match statement below to execute.

ResourceValue::Object(o) => {
let property_bag: Box<dyn PropertyBag> = match &self.value_type {
Some(TypeReference::Named(name)) => {
Box::new(self.schema.type_named(name).cloned().unwrap())
}
Some(TypeReference::Map(item_type)) => Box::new(MapOf(item_type)),
Some(TypeReference::Primitive(Primitive::Json)) => {
Box::new(MapOf(&TypeReference::Primitive(Primitive::Json)))
}
other => unimplemented!("{other:?}"),
};

Due to the unimplemented! macro panicking in-place, a non-descriptive error message is displayed while using cdk migrate:

cdk migrate --from-path './CDKMigrateExampleTemplate.yml' --stack-name CustomStackName
...
 ❌  Migrate failed for `CustomStackName`: stack generation failed due to error 'unreachable'

stack generation failed due to error 'unreachable'

This PR adds a new explicit check for the case where property_type is None. If that case is true, then a more descriptive error message will be displayed explaining what resource property is not valid. For example,

cdk migrate --from-path './CDKMigrateExampleTemplate.yml' --stack-name CustomStackName
...
 ❌  Migrate failed for `CustomStackName`: stack generation failed due to error 'ReadEndpoint is not a valid resource property for resource RDSCluster of type AWS::RDS::DBCluster'

stack generation failed due to error 'ReadEndpoint is not a valid resource property for resource RDSCluster of type AWS::RDS::DBCluster'

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Francis <colifran@amazon.com>
@colifran colifran changed the title chore: add specific error message when an invalid property_type is used chore: add specific error message when an invalid resource property is used Mar 5, 2024
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Merging #594 (15f20dc) into main (2c14390) will increase coverage by 0.2%.
The diff coverage is 94.1%.

Additional details and impacted files
Components Coverage Δ
Parser 76.2% <ø> (ø)
Intermediate Representation 80.4% <94.1%> (+0.9%) ⬆️
Synthesizers 87.3% <ø> (ø)
Other 35.0% <ø> (+0.7%) ⬆️
@@           Coverage Diff           @@
##            main    #594     +/-   ##
=======================================
+ Coverage   81.0%   81.2%   +0.2%     
=======================================
  Files         27      27             
  Lines       5098    5110     +12     
  Branches    5098    5110     +12     
=======================================
+ Hits        4130    4148     +18     
+ Misses       766     761      -5     
+ Partials     202     201      -1     
Files Coverage Δ
src/ir/resources/mod.rs 79.9% <94.1%> (+1.4%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c14390...15f20dc. Read the comment docs.

@colifran colifran changed the title chore: add specific error message when an invalid resource property is used chore(ir): add specific error message when an invalid resource property is used Mar 5, 2024
@TheRealAmazonKendra
Copy link
Collaborator

Could we wordsmith that error a little more? Maybe some thing more like: ${stackName} could not be generated because <the rest of your error message is great>

@TheRealAmazonKendra
Copy link
Collaborator

Neverminddddd, that part of the error message is on the CLI side so approving this since it looks great!

@cdklabs-automation cdklabs-automation added this pull request to the merge queue Mar 5, 2024
Merged via the queue into main with commit 4346c85 Mar 5, 2024
7 checks passed
@cdklabs-automation cdklabs-automation deleted the colifran/improve-error-message branch March 5, 2024 23:58
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Mar 8, 2024
### Reason for this change

This change is a follow-up to a [PR](cdklabs/cdk-from-cfn#594) that improved the error message thrown by `cdk-from-cfn` when an invalid resource property was used in a CloudFormation template. This PR further improves the error message on the cli side.

### Description of changes

Primarily, this PR is a verbiage change. The base error message now states that the `<stack-name> could not be generated because <error-message>`. The error message itself is checked against `unreachable` because any use of `panic!`, `unreachable!`, or `unimplemented!` will cause the `cdk-from-cfn` source code to panic in-place. In the resulting wasm binary, this produces a `RuntimeError` that has an error message of `unreachable`. I've improved this slightly by stating `template and/or language inputs caused the source code to panic`. If the error message is not `unreachable`, then the error message is taken as is with `TransmuteError:` replaced.

Note that we should still continue to improve our error messages in `cdk-from-cfn` by by replacing `panic!`, `unreachable!`, and `unimplemented!` with more detailed error messages.

### Description of how you validated changes

An existing unit test was changed based on the error message verbiage change. Additionally, a new unit test was added to validate that the expected error message would be thrown by the cli when an invalid resource property was used in a CloudFormation template.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants