New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config: use correct YAML marshal func #9712
Conversation
As a bonus, this shrinks the executable size by ~100KB* 🥳 * Binary is ~27MB so nobody will notice 😛 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! 😎
Had a quick peek if there was a specific reason for using this library; I see this was added as part of 7f8bb03, which originally came from docker/compose-cli@7f8bb03 (docker/compose-cli#544), but review on that PR doesn't mention a specific reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (not a maintainer)
The `compose-spec/compose-go` lib is written with `gopkg.in/yaml.v2` as a target. When marshalling via CLI (`compose convert` / `compose config`), we were using a _different_ YAML lib, which was a fork of `go-yaml`, which is what `gopkg.in/yaml.v2` is based off of. Signed-off-by: Milas Bowman <milas.bowman@docker.com>
8b279fa
to
c248e88
Compare
Yup, I'm fairly certain it got auto-imported because it was already available as an indirect dependency at the time! And even if we had been attempting to utilize the custom YAML tag logic it added...it wouldn't work since we weren't ever using it for unmarshaling in the (Sidenote: I spent like 20 mins reading about YAML tags this morning and I still don't really understand them 😵) |
The `compose-spec/compose-go` lib is written with `gopkg.in/yaml.v2` as a target. When marshalling via CLI (`compose convert` / `compose config`), we were using a _different_ YAML lib, which was a fork of `go-yaml`, which is what `gopkg.in/yaml.v2` is based off of. Signed-off-by: Milas Bowman <milas.bowman@docker.com>
What I did
When marshalling via CLI (
compose convert
/compose config
), wewere using a different YAML lib, which was a fork of
go-yaml
,which is what
gopkg.in/yaml.v2
is based off of.The
compose-spec/compose-go
lib is written withgopkg.in/yaml.v2
as a target, so I changed it to use that for marshalling the config.
The fork that was being used was sanathkr/go-yaml.
Looking through
git log
, this was originally introduced transitively byold ECS/CloudFormation code (that now lives in
compose-cli
), so itsusage was likely the result of IDE auto-complete and not intentional.
The only change it added compared to the upstream lib was custom tag
support, which is not something we use (and regardless, since the unmarshal
is done with a different lib, it wouldn't work regardless). See go-yaml/yaml@ed9d249.
Related issue
I discovered this when working on compose-spec/compose-go#298.
(not mandatory) A picture of a cute animal, if possible in relation with what you did