[go: nahoru, domu]

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

Chanjo Dockerfile #218

Merged
merged 4 commits into from
Nov 12, 2020
Merged

Chanjo Dockerfile #218

merged 4 commits into from
Nov 12, 2020

Conversation

northwestwitch
Copy link
Member
@northwestwitch northwestwitch commented Nov 11, 2020

fix #217

This PR adds | fixes:

  • Adds a Dockerfile based on a Linux alpine distro that has already installed conda (so installing Sambamba is easier). It provides also the instructions to install and run Chanjo.

How to prepare for test:

  • run docker build -t chanjo:latest .
  • run the container with : docker run chanjo

Expected outcome:

  • You should see the help from the chanjo command line

Review:

  • Code approved by MM
  • Tests executed by CR

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@northwestwitch
Copy link
Member Author

Test: 🆗

image

@northwestwitch
Copy link
Member Author
northwestwitch commented Nov 11, 2020

This is the super basic docker image. Then I'll try to simulate a database connection using another container in another PR. By the way do we have some test data? @moonso does chanjo work with MariaDB?

@coveralls
Copy link
coveralls commented Nov 11, 2020

Coverage Status

Coverage remained the same at 99.209% when pulling 75e0469 on add_dockerfile into 12e3ec1 on master.

Copy link
Contributor
@moonso moonso left a comment

Choose a reason for hiding this comment

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

Great @northwestwitch !

# Run commands as non-root user
RUN adduser -D worker
RUN chown worker:worker -R /home/worker
USER worker
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
USER worker
USER worker
ENTRYPOINT = ["chanjo"]
CMD = ["--help"]

By doing this one can run the container with

docker run --name chanjo

And then use arguments of course

Copy link
Member Author
@northwestwitch northwestwitch Nov 12, 2020

Choose a reason for hiding this comment

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

Good suggestion! I'll fix it as you say, but without committing exactly your suggestion (you've got an extra = after ENTRYPOINT that breaks the command)

@moonso
Copy link
Contributor
moonso commented Nov 12, 2020

By the way do we have some test data?

Yes there are test data under tests/fixtures

@moonso does chanjo work with MariaDB?

I don't really know, but I guess that if sqlalchemy can run with MariaDB then Chanjo can

@northwestwitch
Copy link
Member Author
northwestwitch commented Nov 12, 2020

@moonso I've fixed as you suggested, adding ENTRYPOINT and CMD and it works. The problem is that when you run the container it executes the commands and then exits (as expected), so you can't for instance execute any other commands on it. Whenever you use the RUN command you are actually creating a new container and it's not what we want with this app right? I think we want the container to persist, so we can execute commands whenever we want. Any suggestion?

@moonso
Copy link
Contributor
moonso commented Nov 12, 2020

Great job @northwestwitch !

No I think that this is exactly the behaviour that we want for a container that is running a cli. Usually they are executed with the parameter --rm which will delete the container after the operation is executed, the image is still there so it is not a big process to create a new container every time a command is run. If it is a web service this if not the case but that should be taken care of in another container, like a nginx container or something.

@northwestwitch
Copy link
Member Author

exactly the behaviour that we want for a container that is running a cli.

Ah ok, then this container does exactly that! Merging! 👍

@northwestwitch northwestwitch merged commit 9bbcb89 into master Nov 12, 2020
@northwestwitch
Copy link
Member Author

Tested by @henrikstranneheim in the MIP workflow and it works:

image

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.

Move to Docker
3 participants