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

Allow extension fields in the v3.4 version of the compose format #452

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Aug 17, 2017

This has been a long requested feature (see docker/compose#2942 and many duplicated issues).

Any top-level key which starts with x- will be ignored by compose. This allows for users to:

  • include additional metadata in their compose files
  • create YAML anchor objects that can be re-used in other parts of the config

This matches a similar feature in the swagger spec definition: https://swagger.io/specification/#specificationExtensions

cc @shin- I'd like to make sure this is consistent with the next version of the v2.x format, so please comment if you think this is not appropriate

@shin-
Copy link
Contributor

shin- commented Aug 17, 2017

Is that preferable to a single, named top level section, like meta or similar? I feel like that would be less confusing / easier to explain. Also, aesthetically speaking x-something is very reminiscent of CSS browser hacks which everyone who's ever had to use them probably really wants to forget.

@dnephin
Copy link
Contributor Author

dnephin commented Aug 17, 2017

They are definitely inspired by css extensions. While they were annoying to use, I think they served their purpose in that it allowed browsers to experiment with features, and features that became popular were eventually incorporated into the official standard (I believe).

We could have a single pre-defined key instead of a pattern. meta to me reads more like it's actually part of "supported" keys, where as x-something I think should make it more obvious that it's ignored by docker and compose. HTTP (I think 1.0) had X-* headers for "extension" headers, but they eventually dropped that recommendation, so there is definitely precedence for the x- prefix from both swagger and http.

Maybe x-ignored or x-extra or something like that? Open to other suggestions as well. Using the pattern x- just makes the naming problem a little easier.

If the concern is documentation, we could document a single x-meta, and let savvy users discover that they can actually use anything.

@tbeadle
Copy link

tbeadle commented Aug 19, 2017

Can this also be added for 2.x config files?

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@codecov-io
Copy link

codecov-io commented Aug 22, 2017

Codecov Report

Merging #452 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
- Coverage   47.02%   47.01%   -0.01%     
==========================================
  Files         198      198              
  Lines       16336    16334       -2     
==========================================
- Hits         7682     7680       -2     
  Misses       8260     8260              
  Partials      394      394

Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM.

It makes sense to have an easy way to have the yaml parser ignore some piece of data.
I'm not sure it's worth it to add a top-level key to namespace these objects.

Would definitely like to hear other maintianers feedback.

@vieux
Copy link
Contributor

vieux commented Aug 24, 2017

I like the way it's done now, so you can have extra keys close to the object they relate to.

Copy link
Contributor

@vieux vieux left a comment

Choose a reason for hiding this comment

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

LGTM

@dnephin dnephin merged commit d83752c into docker:master Aug 25, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 25, 2017
@dnephin dnephin deleted the compose-allow-x-fields branch August 25, 2017 16:37
@dfee
Copy link

dfee commented Aug 25, 2017

Let the trumpets play!

@tbeadle
Copy link

tbeadle commented Aug 25, 2017

Any input on getting this added for 2.x version files? It shouldn't be a breaking change. My company is not able at this time to use 3.x files, but would really like to have this ability.

@shin-
Copy link
Contributor

shin- commented Aug 25, 2017

@tbeadle Yes, it's in our plans 👍

@casperdcl
Copy link

winning

@schmunk42
Copy link

Not very happy with the x- hack beauty but I can live with that.

As said here: moby/moby#31101 (comment) - I'd also have preferred another syntax, ie. .something, but I can also live with it. I just thought to speak this out, since it's not in a stable release yet.

@rdxmb
Copy link
Contributor

rdxmb commented Sep 21, 2017

.something indeed would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet