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

Create Dockerfile and docker's compose.yml file #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dimaguy
Copy link

@dimaguy dimaguy commented May 8, 2022

This PR provides a Dockerfile capable of compiling the source code with just doing DOCKER_BUILDKIT=1 docker build and running it inside of a container.
Together with it is bundled a compose.yml file, which requires Docker CE to be installed (not the same as regular distro provided docker, haven't tested with it though).
This can easily be used to create an image for the end user to pull from the docker registry.

Fixes #13

Copy link
Contributor

@BBaoVanC BBaoVanC left a comment

Choose a reason for hiding this comment

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

This is pretty nice, and will be great for people who are familiar with Docker and want an easy way to run the server. I made a few comments/changes, could you take a look at them please?

Comment on lines +10 to +14
ports:
- mode: ingress
target: 65535
published: "65535"
protocol: tcp
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be shortened:

Suggested change
ports:
- mode: ingress
target: 65535
published: "65535"
protocol: tcp
ports:
- 65535:65535/tcp

Comment on lines +22 to +24
networks:
default:
name: nanolimbo_default
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this section too since it's just the default network.

Suggested change
networks:
default:
name: nanolimbo_default

Comment on lines +8 to +9
networks:
default: null
Copy link
Contributor

Choose a reason for hiding this comment

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

This networks: section should probably be removed since Compose creates one by default itself.

Suggested change
networks:
default: null

context: .
dockerfile: Dockerfile
image: nanolimbo
container_name: nanolimbo
Copy link
Contributor

Choose a reason for hiding this comment

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

This container_name option should be omitted so Docker Compose can pick its own name. Even if you won't be running multiple instances of this service on the same machine in different folders, there's no reason in favor of setting the container name.

See also: https://stackoverflow.com/a/64370490/19003757

Suggested change
container_name: nanolimbo

@dimaguy
Copy link
Author

dimaguy commented Sep 21, 2022

I'm sorry I haven't noticed the comments until now, must've missed in my inbox. I can concede in most comments except the last one, the indentation seems correct in the bind mounts

@BBaoVanC
Copy link
Contributor

BBaoVanC commented Oct 1, 2022

the indentation seems correct in the bind mounts

I guess I've seen it both ways in YAML, and it doesn't really affect the parsing. (I deleted that change just now, I wasn't sure how else to remove the suggestion)

Copy link

@tieb62 tieb62 left a comment

Choose a reason for hiding this comment

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

Should'nt the file for docker compose be named docker-compose.yml instead of just compose.yml ?

@dimaguy
Copy link
Author

dimaguy commented Oct 25, 2022

Should'nt the file for docker compose be named docker-compose.yml instead of just compose.yml ?

docker-compose.yml is legacy naming, currently compose.yml is the go-to given that it is meant to be a more broad standard than just docker

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.

Create a docker image.
3 participants