chore: Allow service manifests and other YAML manifests to be successfully marshalled without losing data

[rclinard-amzn]

• Clarified the comment on Backend Service when it exposes no ports - previously, it suggested that no traffic (inbound or outbound) was possible, while in reality outbound traffic is still allowed.
• Added additional test cases for marshalling various components of service manifests
• Extended most test cases that test the marshalling to also run a round-trip marshal test - checking whether something can be marshalled and then immediately unmarshalled to get the same data back.
• Added the omitempty setting to most yaml tags so that nil/null values aren't placed in the resulting YAML
• Implemented MarshalYAML functions corresponding to every implementation of UnmarshalYAML so that marshalling & unmarshalling behavior is consistent
• Removed an unnecessary implementation of UnmarshalYAML on Image - it did validation that was already done elsewhere with Image.validate()
  • Works around a currently-unreported bug in go-yaml: if you have an inline struct that implements Unmarshaler and call yaml.Marshal on a struct containing that inline struct, the data of the inline struct will get silently dropped
• execute template.New() on demand instead of storing it as a field to allow easier unmarshalling of service manifests as YAML
• Remove some test cases for MarshalBinary implementations that only actually tested implementation details

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

[codecov-commenter]

Codecov Report

:exclamation: No coverage uploaded for pull request base (mainline@faf1182). Click here to learn what that means. The diff coverage is n/a.

@@             Coverage Diff             @@
##             mainline    #3997   +/-   ##
===========================================
  Coverage            ?   68.58%           
===========================================
  Files               ?      249           
  Lines               ?    35163           
  Branches            ?      264           
===========================================
  Hits                ?    24115           
  Misses              ?     9851           
  Partials            ?     1197           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[iamhopaul123]

Same questions as @Lou1415926. Also could you explain why we need to add MarshalYAML?

[rclinard-amzn]

Also could you explain why we need to add MarshalYAML?

A common pattern we have is:

struct StructOrString {
    str *string
    struct *struct
}

Without MarshalYAML, this will get marshalled as something like

structorstring:
    str: "test"

or:

structorstring:
    struct:
        key: val

But, what we want is:

structorstring: test

or:

structorstring:
    key: val

UnmarshalYAML allows us to pick which field of StructOrString the YAML gets unmarshaled into. Similarly, MarshalYAML allows us to pick which field of StructOrString is written in place of the whole object.

[rclinard-amzn]

I've restored the removed tests & added the parser field back - but I've instead made it so that the parser field isn't required to be set, and template.New() will be used if it is not. Hopefully that resolves the concerns with this PR!

[rclinard-amzn]

Thanks for the feedback! Your comment is actually partially addressed in the follow up PR - my follow up PR does some substantial reworks to our service templates to incorporate the newly-added YAML marshaling code! For example, the healthcheck fields are now fully marshaled with yaml.Marshal and then stitched into our service manifests instead of being manually written out in the template. In addition, this code is being tested by our unit tests to make sure it is functional in this PR.

My main concern with merging this PR series into the docker compose branch is that this this PR series is fairly wide-reaching and touches a pretty core part of our codebase. It's very likely to cause conflicts in the future if it's not merged, and I want to make sure that we're testing this code separately since it changes quite a bit! I also want to make sure the docker compose work is focused on docker compose and not fixing dependencies in the rest of the Copilot codebase, since otherwise the diff between mainline and the docker compose branch could quickly become overwhelming!

I can still target this at the docker compose branch, but I think this PR series will be a nice cleanup of mainline to make sure that our MarshalBinary implementations degrade gracefully instead of silently dropping data if we ever tweak the default manifest values. Let me know what you think!

[iamhopaul123]

Hello @rclinard-amzn. I see what you meant. Basically we want the same Marshal logic as how we customize YAML Unmarshal for some fields in the manifest. This makes sense to me.

In terms of whether we should put Marshal in the mainline branch or not. I think I'm inclined to put it into the docker compose branch for now because it won't be an issue for the mainline branch as we don't do Marshal logic there.

since otherwise the diff between mainline and the docker compose branch could quickly become overwhelming!

I agree this is an issue so that we need to act fast and let Copilot customers try docker compose and see how they like it. And then merge those two branches. But I don't think having custom Marshal logic would help that much with this issue, as we can always quickly add them back for every one of them when we merge the branch. Before making the final decision on whether (because we'll see customers reaction) and how we can merge the branch, any code written to the mainline branch could end up being modified a lot in the future and add extra implementation complexity when we introduce new fields to the manifest.

[efekarakus]

Closing the PR for now, we'll resurrect it as we need to introduce MarshalYAML for the manifest 😍