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

docs(README): Add ports mapping to Docker Compose example #1994

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

otaviosilva22
Copy link
Contributor

@otaviosilva22 otaviosilva22 commented Nov 9, 2023

Update Docker Compose example with ports usage, because might be useful.

Description

Update Docker Compose example in readme.md with ports usage.

Motivation and Context

Ports might be useful to expose container to the host machine. This information is very important.

Testing Details

It's only readme.md update.

Example Output(if appropriate)

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

Update docker-compose example with ports usage, because is better and more cofiable than expose usage
@otaviosilva22 otaviosilva22 changed the title Update README.md docs: README.md Nov 9, 2023
@otaviosilva22 otaviosilva22 changed the title docs: README.md docs: docker-compose exame in README.md Nov 9, 2023
@otaviosilva22 otaviosilva22 changed the title docs: docker-compose exame in README.md docs: Docker Compose example in README.md Nov 9, 2023
@LaurentGoderre
Copy link
Member

Maybe have both because they serve different purpose.

Update readme.md include both method to expose the container.
@otaviosilva22
Copy link
Contributor Author

@LaurentGoderre you are right. I added a new commit, what do you think?

@LaurentGoderre
Copy link
Member

LGTM

README.md Outdated Show resolved Hide resolved
add a space

Co-authored-by: Peter Dave Hello <hsu@peterdavehello.org>
@PeterDaveHello PeterDaveHello changed the title docs: Docker Compose example in README.md docs(README): Add ports mapping to Docker Compose example Nov 30, 2023
@PeterDaveHello
Copy link
Member

Please allow me to clarify the PR title.

Copy link
Member

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

@otaviosilva22 do you mind squashing the commits and updating the commit message(Maybe like the PR title)? Otherwise LGTM 👍

@otaviosilva22
Copy link
Contributor Author

@LaurentGoderre, do you know how I can get a aprovall from a maintainer?

@LaurentGoderre
Copy link
Member

@otaviosilva22 I think we can merge after the suggestions from @PeterDaveHello are in

@otaviosilva22
Copy link
Contributor Author

@LaurentGoderre, some checks were not successful.

Run find . -name "*.md" | xargs -n 1 markdown-link-check -c markdown_link_check_config.json -q

ERROR: 1 dead links found in ./README.md !
[✖] https://pkgs.alpinelinux.org/package/edge/main/x86/libc6-compat → Status: 404
Error: Process completed with exit code 123.

What do you suggest?

Broken libc6-compat link fix.
@otaviosilva22
Copy link
Contributor Author

@LaurentGoderre, I fixed libc6-compat link. What do you think?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Peter Dave Hello <hsu@peterdavehello.org>
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit 427eca8 into nodejs:main Feb 14, 2024
2 of 3 checks passed
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.

4 participants