Skip to content
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

marshal: correctly handle empty shell commands #298

Merged
merged 1 commit into from Aug 9, 2022

Conversation

milas
Copy link
Member

@milas milas commented Aug 2, 2022

It's possible to override an image's entrypoint for service
container's by setting service.entrypoint. (Same applies for
image's command.)

In some circumstances, it's desirable to explicitly "clear"
these out from the image. However, currently, if you do specify
entrypoint: '' or entrypoint: [], when the config is
marshaled, that gets lost because of Go default value handling.

Handling this varies slightly between YAML + JSON:

  • YAML: implement IsZeroer interface to specify that only
    nil slices should be omittable & MarshalYAML to ensure
    that nil slices marshal as null rather than []
  • JSON: do NOT use omitempty, meaning that the result will
    always be present in the marshaled output, unfortunately,
    but it will be correct!

This preserves compatibility and "pretty" serialization for
YAML at the expense of some slightly subtle marshalling
semantics.

@milas milas added the bug Something isn't working label Aug 2, 2022
@milas milas requested review from ndeloof, glours and laurazard Aug 2, 2022
@milas milas self-assigned this Aug 2, 2022
@milas
Copy link
Member Author

milas commented Aug 2, 2022

See docker/compose#9634 for more context/motivation

It's possible to override an image's entrypoint for service
container's by setting `service.entrypoint`. (Same applies for
image's command.)

In some circumstances, it's desirable to explicitly "clear"
these out from the image. However, currently, if you do specify
`entrypoint: ''` or `entrypoint: []`, when the config is
marshaled, that gets lost because of Go default value handling.

Handling this varies slightly between YAML + JSON:
 * YAML: implement `IsZeroer` interface to specify that only
   `nil` slices should be omittable & `MarshalYAML` to ensure
   that `nil` slices marshal as `null` rather than `[]`
 * JSON: do NOT use `omitempty`, meaning that the result will
   _always_ be present in the marshaled output, unfortunately,
   but it will be correct!

This preserves compatibility and "pretty" serialization for
YAML at the expense of some slightly subtle marshalling
semantics.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas milas force-pushed the marshal-empty-shell-cmd branch from fba1537 to 666827d Compare Aug 2, 2022
@milas
Copy link
Member Author

milas commented Aug 2, 2022

Reviewers, for your sanity, view with whitespace changes ignored: https://github.com/compose-spec/compose-go/pull/298/files?diff=split&w=1

(types.go has a bunch of fields moving around due to gofmt alignment changes which makes it really annoying to read)

@milas milas marked this pull request as ready for review Aug 8, 2022
@milas milas requested review from nicksieger and ulyssessouza Aug 8, 2022
@milas
Copy link
Member Author

milas commented Aug 8, 2022

Marked this as ready for review - no changes, but I didn't hear any major concerns out-of-band with this approach 🙃

glours
glours approved these changes Aug 9, 2022
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds go to me, just waiting for @ndeloof review to be sure there isn't hidden cases we are missing

ndeloof
ndeloof approved these changes Aug 9, 2022
@glours glours merged commit 5135b47 into compose-spec:master Aug 9, 2022
11 checks passed
@milas milas deleted the marshal-empty-shell-cmd branch Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants