awslabs / goformation

GoFormation is a Go library for working with CloudFormation templates.
Apache License 2.0
846 stars 195 forks source link

Supporting YAML Tags #11

Closed sanathkr closed 7 years ago

sanathkr commented 7 years ago

I have a patch that supports custom tags at https://github.com/sanathkr/yaml/commit/fa6563579c461df233d6367628d5d569c352dcca. It works fairly well in the unit tests I wrote. But I want to test it against the crazy world of CloudFormation templates.

What is the best way for me to swap the original YAML parser with mine?

sanathkr commented 7 years ago

I finally merged my patch into a fork of goformation and pushed changes to https://github.com/sanathkr/goformation/tree/0.1.0.

Test failures

I uncommented intrinsic tags test and see the following failure

ERROR: json: cannot unmarshal object into Go struct field Properties.Runtime of type string

for template:

AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31
Description: SAM template for testing intrinsic functions with YAML tags
Resources:
  IntrinsicFunctionTest:
    Type: AWS::Serverless::Function
    Properties:
      Runtime: !Sub "test-${runtime}"

The struct definition expects Runtime to be a string but YAML document has a dictionary - {"Sub": "test-${runtime}"}

@PaulMaddox I thought the intrinsics resolver will run before JSON unmarshalling completes, right?

sanathkr commented 7 years ago

My bad, just realized Sub needs to have prefix Fn::. Will fix it now

sanathkr commented 7 years ago

Intrinsics Tags Works!! 🎉

Pushed latest code to https://github.com/sanathkr/goformation/tree/0.1.0

Running Suite: Goformation Suite
================================
Random Seed: 1503106816
Will run 45 of 45 specs

•••••••••••••••••••••••••••••••••••••••••••••
Ran 45 of 45 Specs in 0.000 seconds
SUCCESS! -- 45 Passed | 0 Failed | 0 Pending | 0 Skipped PASS
ok      github.com/awslabs/goformation  0.090s

Following template resolves the Timeout parameter like a charm:

Parameters:
  TimeoutParam:
    Type: Number
    Default: 10
Resources:
  IntrinsicFunctionTest:
    Type: AWS::Serverless::Function
    Properties:
      Runtime: !Sub "${ParamNotExists}4.3"
      Timeout: !Ref TimeoutParam

What's not working?

When Timeout: !Ref NonExistentParam then I get the error: ERROR: json: cannot unmarshal string into Go struct field Properties.Timeout of type int

@PaulMaddox Thoughts?

PaulMaddox commented 7 years ago

Awesome!

I'll do some testing.

Currently if GoFormation can't resolve a !Ref, it just returns the reference name: https://github.com/awslabs/goformation/blob/0.1.0/intrinsics/ref.go#L56

I think this was an oversight on my part, and actually if it returned nil then you wouldn't see that type error and the template would marshal OK.

sanathkr commented 7 years ago

Makes sense. Still nil wouldn't be assignable to an int right? We might have to look at the type of the object and assign defaults, if that's the case.

I have plenty more SAM templates. I will pull them here for testing.

PaulMaddox commented 7 years ago

I've also realised that the implementation of Fn::Sub in the intrinsics package currently only does string replacements, it doesn't lookup references which the real Fn::Sub does.

I suggest we get this into the 0.1.0 branch on the main goformation repo, then i'll start working on that.

PaulMaddox commented 7 years ago

Actually, I think the JSON unmarshaller will be able to marshal nil into int (i think), as we have the https://golang.org/pkg/encoding/json/ flag set for all parameters.

PaulMaddox commented 7 years ago

Yep, have written some tests that confirm that nil is successfully marshalled into an int :)

sanathkr commented 7 years ago

Omit empty will omit required properties if it contains an unresolvable reference.

Like:

MyFunction:
  Event: 
     MyApi:
        Type: Api
        Path: !Ref GetPath

where GetPath is a parameter without default.

We can workaround this by allowing customers to specify an optional JSON of parameter values that we use to resolve the intrinsics. We can get this implemented if this becomes a problem.

@PaulMaddox We certainly should document OmitEmpty and its behavior.