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

Add support for docker installation #31

Merged
merged 8 commits into from
Oct 21, 2018

Conversation

mostafahussein
Copy link
Contributor

This PR adds support for installing joomscan as a docker container

@Ali-Razmjoo
Copy link
Collaborator

great contribution, @rezasp please consider this PR.

@Ali-Razmjoo
Copy link
Collaborator

Hi @mostafahussein,

once again thanks for your contribution, it's a great idea to have dockerfile on this project, here are my comments on this PR.

  • dockerfile works fine, but please add a new line to and mount reports folder to /home/joomscan/reports and let users access the report files through the host machine. users will not be able change entrypoint to /bin/bash without modifying the dockerfile.
  • since we have opensource license and everyone are welcomed to contribute this project, this line is unnecessary LABEL maintainer="Mostafa Hussein <[email protected]>" please remove it.

Thank you.

@mostafahussein
Copy link
Contributor Author

@Ali-Razmjoo , Can I change it to author instead of maintainer ?

@Ali-Razmjoo
Copy link
Collaborator

@rezasp should tell about this policy, in other projects usually contributor can add their name as a comment in the code, but Reza please guide @mostafahussein regarding this project.

@mostafahussein
Copy link
Contributor Author

mostafahussein commented Oct 21, 2018

for the /home/joomscan/reports part it can be added during creating a new container as the following:

docker run -v /path/on/host:/home/joomscan/reports --name joomscan_container rezasp/joomscan

This will makes you able to access the reports through the host, also you can use docker-compose as it provides an easier way to handle docker instead of the docker cli itself

In order to modify the entrypoint without modifying the dockerfile you can do the following:

docker run -v /path/on/host:/home/joomscan/reports --name joomscan_container --entrypoint /bin/bash rezasp/joomscan

I think it will be suitable to add these examples in the readme file itself without modifying the current dockerfile. what do you think ?

Update:
I will provide a docker-compose.yml file which builds the image and mounts the reports directory on the host.

@Ali-Razmjoo
Copy link
Collaborator

I talked to Reza, he also confirmed, it's ok to add your name as a comment in Dockerfile. in that case please modify the readme file and add the docker section.

@mostafahussein
Copy link
Contributor Author

@Ali-Razmjoo , I have updated the dockerfile and the readme kindly check it and let me if there is any modification needed

@Ali-Razmjoo
Copy link
Collaborator

@mostafahussein thanks for your contribution.

@rezasp it's ready to merge. please consider.

@rezasp rezasp merged commit 60f7446 into OWASP:master Oct 21, 2018
@rezasp
Copy link
Collaborator

rezasp commented Oct 21, 2018

Hello,

Thank you for your contribution, PR Merged.

Best Regards.

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.

3 participants