defork goformation

[errordeveloper]

We've used a fork of goformation since #132.

The initial attempt of contributing our changes back to goformation (awslabs/goformation#103) didn't succeed, and goformation maintainers chose a different approach (awslabs/goformation#114). However, their solution is not bi-directional, has a number of limitations/bugs (e.g. awslabs/goformation#190 and awslabs/goformation#194).

The main challenge is that mapping CloudFormation scheme to Go types is very complicated if you consider a general case and support of all types is required as well as Serverless Application Model (SAM), and that is what goformation attempts.
However, we only used a relative small number of types in a particular way.

It appears to be easy enough to create a slim library that offers a subset of goformation functionality that does things the way that suites our needs better without having to take all general cases into account. Aside from being able to round-trip templates from running stacks back to internal structures for mutation and updates, we could provide convenient abstractions for testing of template rendering.

[michaelbeaumont]

I've decided the best way is not to use the vanilla newest version of goformation. I have a 99% working version of eksctl with goformation@v4.11.0 but I'm not happy with it. The alternative chosen back then by goformation is a really fragile way of doing things, where they embed intrinsics as base64 strings.
It seems mostly to have been done in order to avoid breaking changes. The API is only "simpler" because all intrinsics become opaque strings and a pass is done on (un)marshaling in order to convert the strings back, which very much complicates the implementation. As mentioned in the description, these decisions prevent one from parsing a template back into go.

If I have:

type Thing struct {
  Field string
}

func Ref(f string) string {}

t := Thing{
  Field: Ref("OtherField")
}

there's no way for me to Unmarshal(Marshal(t)) and get the same result back unless Field contains the opaque base64 string after Unmarshaling.

It looks like they tried to alleviate this by "resolving" intrinsics, but this is fragile, incomplete and it's unclear to me why anyone would want this; (un)marshaling is no longer anything close to an isomorphism.

I tried rebasing the fork's changes on the newest version and am nearly done and I think this will work just as well as the fork did. We can always regenerate the types to get the newest changes. We could potentially even disable support for things we don't need. It ultimately can serve as the slimmed down library imagined above.

[errordeveloper]

 The alternative chosen back then by goformation is a really fragile way of doing things, where they embed intrinsics as base64 strings.

Hi @michaelbeaumont :) that is exactly what I made of it at the time.

Have you considered extending the pkg/cfn/template? I thought it was coming out quite nicely, given the usage patteren, it seemed like just the right fit.

[michaelbeaumont]

Hi @errordeveloper! It's based on the same type you came up with in the fork, Value, right? I think it's the right API.
I rebased and extended your Value changes on top of the latest goformation so that we can also parse templates back into go which seems to be working and made at least one eksctl test cleaner.

With pkg/cfn/template we can narrow types to precisely what we need for specific cfn resources (string vs *Value). But then we have to maintain the types.

With the goformation fork, everything is generated, but the generation code is relatively complicated. Otherwise, given the pace of development in goformation I don't anticipate having to take action for any changes upstream except running go generate to get cfn updates.

Since both options have Value in common, I suppose the ultimate question is do we maintain our own types or do we maintain the generation code in the fork?
Do you see any other advantages/disadvantages? What do you think?

[errordeveloper]

Hey @michaelbeaumont!

I have indeed copied most of the code from the fork into cfn/template, so Value is largely the same thing.

I think the set of types doesn't grow all that often. Since the usage is very specific, it seems to be right to make opinionated choices of what fields are always just strings Vs Value, and some fields just don't get used at all.

One theoretical use-case to consider - arbitrary CloudFormation resource customisation/inclusion. Personally, I don't think this could ever work in eksctl, not without major generalisation of all the things, so I don't see that as a feasible use-cases, albeit it seems plausible in theory, and perhaps to some users.

Did you have a look at the tests in pkg/cfn/template and the tests for its main consumer (pkg/cfn/builder/iam_test.go, if my memory serves me well)? I do recall those where quite nice and high-level, with gomega matchers etc.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.