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

Update README.md #1

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

Update README.md #1

wants to merge 2 commits into from

Conversation

adrianvu
Copy link

@adrianvu adrianvu commented Dec 5, 2017

Previously had an error connection refused. Fix "-p 8080:80" to "-p 80:80". Port 80 instead of 8080 was exposed in Dockerfile. Now it should work when trying to access from outside the container.

Previously had an error connection refused. Fix "-p 8080:80" to "-p 80:80". Port 80 instead of 8080 was exposed in Dockerfile. Now it should work when trying to access from outside the container.
@bylexus
Copy link
Owner

bylexus commented Dec 6, 2017

I do not agree: The documentation is meant to show how to map another Host's port to the internal port 80. This is common practice if you have multiple containers running on the same host.
Of course you CAN map it to the host's standard port 80, but you don't have to.

Copy link
Owner

@bylexus bylexus left a comment

Choose a reason for hiding this comment

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

  • I do not agree to the README.md file change: It is intentional that this is documented as Port 8080:80, as this is the common use case for docker containers. Please remove this change.

  • Agree to the Dockerfile change, I will accept this change, but please create a new pull request: You're mixing 2 changes under the same title here.

--> Please change this request as suggested:

  1. Remove the README.md change
  2. Rename the change request, as the title now says "Updated README.md"

Thanks

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.

None yet

2 participants