#60 Docker set up for development enviroment
Closed by gabrielsclimaco. Opened by gabrielsclimaco.
kiskadeemes/kiskadee enviroment-with-docker  into  api

Download 60.patch

What this PR do:

  • Add a Dockerfile to build an image for kiskadee development environment
  • Add a shell script to run the built image
  • Update README.md with a "how to" to use de docker

What issues does this PR closes:
#59 Docker set up for development environment

Hi @gabrielsclimaco ,

I have a few considerations on this PR:

  • A Dockerfile for development seems to be a good idea, thanks :)

  • Your branch seems to be a little behind master. Please, rebase it before submitting the patch

  • I don't think we need a shell script to run the container. Any thoughts here?

rebased

gabrielsclimaco commented

Hey, about your comments:

  • As the PR aims branch api I rebased the source branch from api
  • I created a shell script for two main reasons:
    • Because the command used to run the docker is very extensive. With a shell script, all it needs to do it's ./run_docker.sh.
    • With a shell script I can document the command used to run the docker.

rebased

Thanks for the changes!

  • There is no need to add comments describing your programs step-by-step, as you did in the shell script. A programmer should be able to read and understand your code.

  • I Still believe that the run_docker.sh script does not add that much value there, it might be confusing, I myself have no intentions to run kiskadee using docker in that way for now. You could do that in a Makefile though: We could move other things there, like building the containers for all the runners (which is a shell script for now).

  • Is there any reason for wanting to merge this in the api branch? this could go straight to master.

  • There is still some issue with the PR, it still proposes a merge with some other commits, make sure you are applying your changes on HEAD of whatever branch you want us to merge it.

Pull-Request has been closed by gabrielsclimaco

gabrielsclimaco commented

Opened as a new one targeting master https://pagure.io/kiskadee/pull-request/61/

Metadata