Issue #134 - FN::GetAtt can return an array #139
Conversation
src/test/validatorTest.ts
Outdated
| expect(result['errors']['warn']).to.have.lengthOf(0); | ||
| }); | ||
|
|
||
| it("should not pass validation where Fn::GetAtt returns a list", () => { |
There was a problem hiding this comment.
No need for the JSON versions, they look like they are testing the same stuff as the YAML?
There was a problem hiding this comment.
OK - I had assumed that the processing for Fn::GetAtt and !GetAtt might be different, and included tests for both. Happy to strip them out.
src/validator.ts
Outdated
| return `mockAttr_${reference}_${attributeName}`; | ||
| } else { | ||
| // Lookup attribute | ||
| const attribute = resourcesSpec.getResourceTypeAttribute(resource['Type'], attributeName) |
There was a problem hiding this comment.
We need to catch the exception and throw it back to the user as a Critical error...
Also need a test case for this type of behaviour, for example requesting "!GetAtt DNSVPCDomain.VeryLostNameServers"
should result in:
Resource: Resources > .....
Message: Required property ...
Documentation: ...
Template invalid!
Such examples are
addError('crit', `Fn::If does not allow ${keys[0]} as a nested function within an array`, placeInTemplate, 'Fn::If');
Probably: Fn::GetAtt cannot find property AAAA on resource AAAA of type AAAA, placeInTemplate, DestinationType?? (if possible), otherwise fn::getatt
The last parameter decides which documentation to serve to the user.
There might be some better ones within the validator
|
Ok - give that a go. I'm unsure if you really want exception processing in that function, but it does work. For the returned documentation, I'd thought what would be most useful - the type on which the attribute cannot be found. |
|
Agreed about the exception processing, it doesn't really go with the style of the rest of the program. However, the end result is what we expect. Maybe having a Happy with resource type, looks good! |
|
Is it enough of a wart to fix right now? I'd be happy to help with the refactor as and when - my own CoffeeCloud (https://github.com/blackravenltd/CoffeeCloud) project is hitched to the |
| // Lookup attribute | ||
| const attribute = resourcesSpec.getResourceTypeAttribute(resource['Type'], attributeName) | ||
| const primitiveAttribute = attribute as PrimitiveAttribute | ||
| if(!!primitiveAttribute['PrimitiveType']) { |
There was a problem hiding this comment.
Yup, that's my oldskool JS showing itself... 'double-not' coerces to boolean (i.e. true if not falsy). The intent here is to test for the existence of the property and make sure it has a value - I could have used hasOwnProperty (or instanceof) come to think of it, unless there is an idiomatic TypeScript way to do it...
|
Looks like the build is failing due to a new npm version, will merge this and push a new release when resolved 😀 |
First off, my TypeScript isn't that good, corrections welcome. This works but as always I invite discussion/critique.
getResourceTypeAttributeinresourcesSpec.tsthat gets the named attribute on the specified resource type, or throwsNoSuchResourceTypeAttributeif not found.ItemTypeonPrimitiveAttributetoPrimitiveTypefnGetAttto get the Attribute usinggetResourceTypeAttributeand return a List of mocks if specified.getResourceTypeAttributefnGetAtt