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 map service ports option for run command. #485

Merged
merged 1 commit into from Dec 19, 2014

Conversation

Projects
None yet
@stcqu
Copy link
Contributor

stcqu commented Sep 16, 2014

When using the fig run command, ports defined in fig.yml can be mapped
to the host computer using the -m or --map-ports option. Resolves #163.

Signed-off-by: Stephen Quebe squebe@gmail.com

@stcqu

This comment has been minimized.

Copy link
Contributor

stcqu commented Sep 16, 2014

I missed pull #251 that solves the same issue. That should be referenced as well.

@bfirsh

This comment has been minimized.

Copy link
Contributor

bfirsh commented Sep 16, 2014

Looks great. I wonder if --service-ports would be a better name for the option. I'm not sure we should add a short option too – it'll collide with the docker run options.

@esnunes

This comment has been minimized.

Copy link

esnunes commented Sep 16, 2014

I thought the idea of fig file was to describe the "running" environment of a container, what is the good reason of not mapping ports by default?

I might got it wrong, but in case I would like not to map ports I would create a new item on yml file without the port mapping.

@stcqu

This comment has been minimized.

Copy link
Contributor

stcqu commented Sep 17, 2014

I agree that it would reduce confusion to have both run and up map service ports by default. Although, either way, I think it's important to have an option for the run command. I just removed the short option and added a separate commit to rename --map-ports to --service-ports. I think map ports is a little clearer, but they both make sense to me.

@gregory

This comment has been minimized.

Copy link

gregory commented Oct 14, 2014

agree with @esnunes ! crazy that no one complained before :)
Mapping should be the default behavior.

@klochner

This comment has been minimized.

Copy link

klochner commented Oct 14, 2014

or even better:

  • map by default
  • give option to not map with flag "--no-ports"
  • rescue on port conflict and suggest using the no-ports option
@gregory

This comment has been minimized.

Copy link

gregory commented Oct 14, 2014

+1 for @klochner !

@kikicarbonell

This comment has been minimized.

Copy link

kikicarbonell commented Nov 18, 2014

:+1 for @klochner, +1 - this is important for debugging live apps

@klochner

This comment has been minimized.

Copy link

klochner commented Nov 18, 2014

I've given up on fig for development until this is resolved.

@gregory

This comment has been minimized.

Copy link

gregory commented Nov 18, 2014

you should have a look at crane @kikicarbonell

@nonsensery

This comment has been minimized.

Copy link

nonsensery commented Nov 26, 2014

Who do I have to should I buy 🍻 for in order to get this merged? I love using Fig, but not being able to debug my Rails app is going to get old really quickly.

Maybe mapping ports should be the default behavior, but it was obviously an intentional choice to have things the way they are, so it seems more likely to get this merged as an opt-in behavior.

I've done the work to resolve the merge conflicts in this branch. Would it be helpful to create a new PR from that branch? I'm happy to take it over if you want, @squebe.

@dnephin

This comment has been minimized.

Copy link
Contributor

dnephin commented Dec 3, 2014

This LGTM, we should get this into the next release.

Please rebase with master and squash to a single commit, then it should be all set.

@dnephin dnephin referenced this pull request Dec 6, 2014

Closed

Run tmux command #710

Added map service ports option for run command.
When using the fig run command, ports defined in fig.yml can be mapped
to the host computer using the -service-ports option.

Signed-off-by: Stephen Quebe <squebe@gmail.com>
@stcqu

This comment has been minimized.

Copy link
Contributor

stcqu commented Dec 12, 2014

@dnephin @nonsensery I just rebased and squashed it to a single commit. Let me know if there more to do.

@dnephin

This comment has been minimized.

Copy link
Contributor

dnephin commented Dec 12, 2014

LGTM

@nonsensery

This comment has been minimized.

Copy link

nonsensery commented Dec 18, 2014

Nice, can't wait to see this hit the release channel!

aanand added a commit that referenced this pull request Dec 19, 2014

Merge pull request #485 from squebe/run-service-ports
Added map service ports option for run command.

@aanand aanand merged commit 0e17222 into docker:master Dec 19, 2014

1 check passed

continuous-integration/wercker Build finished
Details
@klochner

This comment has been minimized.

Copy link

klochner commented Dec 19, 2014

sweet, nice work guys

@viatcheslavmogilevsky

This comment has been minimized.

Copy link

viatcheslavmogilevsky commented Dec 24, 2014

Nice, can't wait to see this hit the release channel! [2]

@karellm

This comment has been minimized.

Copy link

karellm commented Dec 29, 2014

Really looking forward to it. Any ETA for the release?

@zephraph

This comment has been minimized.

Copy link

zephraph commented Dec 29, 2014

I'm glad to see this finally implemented. I actually can't use fig until this is released.

@kevinpostal

This comment has been minimized.

Copy link

kevinpostal commented Jan 8, 2015

ETA? For release?

@tebanep

This comment has been minimized.

Copy link

tebanep commented Jan 9, 2015

Any ETA? This is a must for development. Thanks!

@karellm

This comment has been minimized.

Copy link

karellm commented Jan 10, 2015

It looks like 1.1 is coming and I know it is annoying to be harrassed for a release date. That being said, this is an issue that's been running for about 4 months and been solved for about a month.

Could you consider changing your release cycle to release things as they are made/solved rather than making big bundle of somewhat unrelated features like 1.1 seems to be?

Thanks!

PS: this is really a blocking issue in my daily use of Fig, and looks like one for many people.

@stcqu stcqu deleted the stcqu:run-service-ports branch Jan 10, 2015

@gregory

This comment has been minimized.

Copy link

gregory commented Jan 10, 2015

just use crane!

@tebanep

This comment has been minimized.

Copy link

tebanep commented Jan 12, 2015

I finally realized that @gregory is completely right. Just use crane! It's easy and works as one would expect. Give it a try.

@szuliq

This comment has been minimized.

Copy link

szuliq commented Mar 11, 2015

Can anyone comment on why this isn't a default option? Sure, now with command line completion it's fairly easy to type that. The problem is the person need to know about this option.

In my opinion—if that's possible—what @klochner suggested would benefit newcomers.

@nonsensery

This comment has been minimized.

Copy link

nonsensery commented Mar 11, 2015

I think the reason it's not on by default is for backwards compatibility.

Since only one container can use a port at a time, you can't use "run --service-ports" while that service is "up" (or while any other run-with-ports is in progress).

~ Alex

On Mar 11, 2015, at 1:52 AM, Krzysztof Szularz notifications@github.com wrote:

Can anyone comment on why this isn't a default option? Sure, now with command line completion it's fairly easy to type that. The problem is the person need to know about this option.

In my opinion—if that's possible—what @klochner suggested would benefit newcomers.


Reply to this email directly or view it on GitHub.

@aanand

This comment has been minimized.

Copy link
Contributor

aanand commented Mar 13, 2015

Since only one container can use a port at a time, you can't use "run --service-ports" while that service is "up" (or while any other run-with-ports is in progress).

This, exactly. The default case we're serving is running a command in a web container while another web container is running, (e.g. doing a database migration, opening up a console, etc).

yuval-k pushed a commit to yuval-k/compose that referenced this pull request Apr 10, 2015

Merge pull request docker#485 from squebe/run-service-ports
Added map service ports option for run command.
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>

@bfirsh bfirsh added kind/enhancement and removed enhancement labels Jul 16, 2015

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