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

Add override option to service configuration for ports #6213

Closed
wants to merge 1 commit into from

Conversation

jovobe
Copy link

@jovobe jovobe commented Sep 21, 2018

This adds the ability to explicitly override the ports configuration with a given docker-compose.override.yml. This is manly an updated version of this PR: #3939

Problem

We have a docker-compose.yml which we want to modify by using a docker-compose.override.yml. According to the docs [1] the ports section is overridden as follows:

For the multi-value options ports, expose, external_links, dns, dns_search, and tmpfs, Compose concatenates both sets of values:

This is bad in some cases: What if the base file contains a port binding which we want to change? This is currently not possible.

Solution

Inspired by @CarstenHoyer, I used his idea to add a new field to the service configuration called override. In this field all override-fields can be set (currently only ports is supported). This causes docker-compose to ignore the port configuration from the base and only uses the override port configuration.

Example

docker-compose.yml

version: '2.1'
services:
  web:
    image: example/web
    ports:
      - "80:80"

docker-compose.override.yml

version: '2.1'
services:
  web:
    ports:
      - "8080:80"
    override:
      - ports

Before this PR the result would be:

version: '2.1'
services:
  web:
    image: example/web
    ports:
      - "80:80"
      - "8080:80"

with this PR the result will be:

version: '2.1'
services:
  web:
    image: example/web
    ports:
      - "8080:80"

Signed-off-by: Johan M. von Behren johan@vonbehren.eu

Resolves #2260 and #3729


[1]: https://docs.docker.com/compose/extends/#adding-and-overriding-configuration

This adds the ability to explicitly override the ports configuration
with a given docker-compose.override.yml.

Signed-off-by: Johan M. von Behren <johan@vonbehren.eu>
@nyetwurk
Copy link

nyetwurk commented Sep 24, 2018

As a side note, "ports" is an interesting case. I don't think

ports:
 - "80:80"
 - "8080:80"

has any utility anyway (even if it does have a logical meaning). After the first xx:80, subsequent yy:80s should override any previous ones...

The ports: syntax is kind of screwy in this regard; it should be a dict where the second port is the key for the first port as a value, rather than just a list of strings.

It is probably too late to revisit the ports keyword, but perhaps a separate keyword that indicates a dict with port mappings would be more useful, and would not require strange override behavior.

e.g.:

portmap:
  - 80=80
  - 22=22

could be overridden with

portmap:
  - 80=8080

resulting in

portmap:
  - 80=8080
  - 22=22

Not sure how to go about deleting a port mapping. Perhaps - 22=0 or - 22="" would remove - 22=22 and result in

portmap:
  - 80=80

@jovobe
Copy link
Author

jovobe commented Oct 1, 2018

Any feedback on this @shin-?

@shin-
Copy link

shin- commented Oct 2, 2018

Thanks @jovobe for picking up and updating the previous PR.

My issue with this and the previous proposals is that I find the design extremely confusing and UX extremely poor (same for #5354). If we're going ahead with such a significant change, I want us to get it right, and I don't think we've found the right answer yet.

If you're invested in getting this resolved, would you be willing to create an issue dedicated to discussing the design for this feature? I'd start with an RFE to formalize what we're trying to achieve, and then we can start brainstorming what that looks like concretely in a Compose definition.

@nyetwurk
Copy link

nyetwurk commented Oct 2, 2018

I agree, and still think the solution is to move away from an array and towards a dict, which provides the end user with all possible override possibilities (without requiring an override directive), except for adding multiple external port mappings for the same internal port, which, IMO isn't really useful in this context anyway.

@shcallaway
Copy link

I'll admit that my PRs (#5354 and #5355) are lacking on the design/UX side. I just wanted to get something out there in order to see if there's demand. I'm going to close those PRs for the time being. At some point, I'll consider revisiting this feature. :)

@ijc
Copy link

ijc commented May 20, 2019

Was a design issue (as proposed in #6213 (comment)) ever opened?

Coming at this now it seems to me that this is a usecase which would be best served these days by docker app where you would simply make the port a parameter.

Given both of those I'm going to close this PR for now, if docker app doesn't satisfy everyone's needs (from what I can see it does satisfy the authors) I would suggest opening a design issue for your particular use case either here on on the docker/app repo.

@ijc ijc closed this May 20, 2019
@nyetwurk
Copy link

docker app does not satisfy anything.

@nyetwurk
Copy link

What purpose does closing this serve?

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

Successfully merging this pull request may close these issues.

Overriding the set of values when extending the "ports" option.
6 participants