CFn: improve error message for invalid ref #10149
Conversation
simonrw
added
the
semver: patch
dominikschubert
requested review from
dominikschubert and
pinzon
as code owners
| @@ -169,7 +169,9 @@ def resolve_ref( | |||
| # resource | |||
| resource = resources.get(ref) | |||
| if not resource: | |||
| raise Exception("Should be detected earlier.") | |||
| raise Exception( | |||
| f"Resource target for `!Ref {ref}` could not be found. Is there a resource with name {ref} in your stack?" | |||
There was a problem hiding this comment.
*with the logical resource id
There was a problem hiding this comment.
I was trying to not be too technical in this message. I think referring to "name" is more actionable, rather than "logical resource id" which is technically correct.
| raise DependencyNotYetSatisfied(resource_ids=resource_logical_id, message="") | ||
| raise DependencyNotYetSatisfied( | ||
| resource_ids=resource_logical_id, | ||
| message=f"Could not resolve attribute {attribute_name} on resource {resource_logical_id}", |
There was a problem hiding this comment.
nit: enclosing replaced values with quotation marks might make this a bit more readable
| @@ -361,7 +366,7 @@ def _resolve_refs_recursively( | |||
| none_values = [v for v in join_values if v is None] | |||
| if none_values: | |||
| raise Exception( | |||
| "Cannot resolve CF fn::Join %s due to null values: %s" % (value, join_values) | |||
| f"Cannot resolve CF Fn::Join {value} due to null values: {join_values}" | |||
There was a problem hiding this comment.
Here we're using the full canonical version while using !Ref above. Would suggest keeping this consistent. (Personal preference would be slightly towards canonical)
|
|
||
|
|
||
| @markers.aws.only_localstack | ||
| def test_useful_error_when_invalid_ref(deploy_cfn_template): |
There was a problem hiding this comment.
Good idea to add a test here tp avoid us messing it up when refactoring on the future. We might want to pull out tests like this from the file in the future though.
Motivation
When we cannot resolve a
Ref- specifically if it is targeting a non-existent resource or parameter, we currently print "Should be detected earlier." which is not very useful. Instead, since this is a programming error from the creator of the template, we should be more helpful and print out the ref that cannot be resolved.Changes