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

Added "scope" to docker-cli compose #420

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

glyif
Copy link

@glyif glyif commented Aug 6, 2017

Fixes #410

- What I did
I updated the docker-cli's compose to be able to take in scope as a network key with a value of swarm or local. feature request for : #410

- How I did it
I updated the config_schema, added the scope to the Network Config struct, generated new bindata, edited tests to test for the scope.

- How to verify it
This currently only works with the --compose-file flag in the docker-cli, but doesn't work with docker-compose yet. I will be working on that next

- Description for the changelog
I updated the config_schema, added the scope to the Network Config struct, generated new bindata, edited tests to test for the scope.

- A picture of a cute animal (not mandatory but encouraged)
My dogs, Hex the Husky (left) and Linux the Aussie (right).

20170630_204546

@codecov-io
Copy link

codecov-io commented Aug 6, 2017

Codecov Report

Merging #420 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   52.96%   52.96%   +<.01%     
==========================================
  Files         244      244              
  Lines       15828    15829       +1     
==========================================
+ Hits         8383     8384       +1     
  Misses       6891     6891              
  Partials      554      554

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

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 🐸
/cc @vieux @dnephin same question as before, Is 17.08 freeze in place ? if yes, the schema modification should target 3.5 instead of 3.4.

@dnephin
Copy link
Contributor

dnephin commented Aug 22, 2017

afaik we're still good for 3.4

@thaJeztah
Copy link
Member

@glyif looks like this needs a rebase

@dnephin @vdemeester is this ready to go other than a rebase?

@vdemeester
Copy link
Collaborator

@thaJeztah @dnephin I think we missed the 3.4 window 😅 but other than that we are good to go 👼

@dnephin
Copy link
Contributor

dnephin commented Sep 26, 2017

I think so, this will need to be moved to the 3.5 schema since 3.4 has been released, but we can do that as a follow up if necessary

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester
Copy link
Collaborator

vdemeester commented Jan 26, 2018

@dnephin @thaJeztah @glyif Added a 3.7 schema version (as 3.6 is already on 18.02) and rebased this PR 👼

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@vdemeester
Copy link
Collaborator

cc @shin- for docker/compose 👼

Signed-off-by: glyif <122@holbertonschool.com>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@dnephin
Copy link
Contributor

dnephin commented Jan 26, 2018

Hmm, thinking about this again. Are we sure this is something we want to expose in the Compose file?

If someone uses scope: local will the services actually be able to connect to it? I guess only if the service is on the same node?

@thaJeztah
Copy link
Member

moving back to design review

ping @fcrisciani WDYT?

@fcrisciani
Copy link

@thaJeztah host, none, bridge are all only local networks, overlay is only swarm scope and macvlan can be both, but in the case of swarm scope a config is going to be needed for each node. Not super clear to me how these possible scenarios are actually handled with a simple compose

@dankolesnikov
Copy link

Will this be merged anytime soon? Thx!

@unixfox
Copy link

unixfox commented Dec 6, 2019

Would love to see this PR merged!

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.

Option to specify network scope in compose file
9 participants