Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

docs: Updated the Docker setup for portal #610

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

husainattar
Copy link
Contributor

Description

Updated the docker-compose.yml,settings.py, Dockerfile and Readme.md
for easy project installation process through docker.

Fixes # 581

Type of Change:

Delete irrelevant options.

  • Code
  • Documentation

Code/Quality Assurance Only

  • Bugfix (non-breaking change which fixes an issue)

How Has This Been Tested?

portaldocker

Checklist:

Delete irrelevant options.

  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Code/Quality Assurance Only

  • My changes generate no new warnings

@sakshi1499
Copy link

@husainattar There are failing tests, please push the changes to these.

@husainattar
Copy link
Contributor Author

Hi @sakshi1499 actually I have not much idea with TRAVIS-CI and I have somehow resolved the failure regarding pull request build but I don't know how to resolve the failure regarding CHECK COMMIT MESSAGES.

@sakshi1499
Copy link

@husainattar Actually you have 15 commits and one of them doesn't follow the Commit Guidelines so squash them and the new commit should follow the commit guideline.

@husainattar
Copy link
Contributor Author

@sakshi1499 ohk so I should squash all the 15 commits and create a single one commit as per guidelines?

theyashshahs
theyashshahs previously approved these changes Jul 24, 2020
Copy link
Contributor

@theyashshahs theyashshahs left a comment

Choose a reason for hiding this comment

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

follow the commit style guidelines

@SanketDG
Copy link
Contributor

@husainattar There is an issue with the recent commit message checker, it does not allow capital letters in the subject

#624 should fix this.

@husainattar
Copy link
Contributor Author

Ohh Thank you @SanketDG

@theyashshahs theyashshahs added Status: Changes Requested Changes are required to be done by the PR author. Status: Needs Review PR needs an additional review or a maintainer's review. labels Jul 26, 2020
@SanketDG
Copy link
Contributor

SanketDG commented Jul 27, 2020

@husainattar Feel free to squash the commits together! That should let go of the errors.

@husainattar husainattar force-pushed the develop branch 2 times, most recently from b253b4c to 743f8b5 Compare July 27, 2020 20:57
@husainattar
Copy link
Contributor Author

Hi @SanketDG sir I actually tried everything and also squash the commit but still it provides me check-commits error. I would be grateful if anyone can provide help in this.Thank you

@SanketDG
Copy link
Contributor

@husainattar Should be fixed now! I will review this PR some time later.

@husainattar
Copy link
Contributor Author

Thank you @SanketDG sir thank you for the help.

Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

This looks good, except for some minor comments.

I will also try fixing the docker documentaiton, so that it's not just a preview, but usable as a "development environment", so that a lot of the problems regarding Installation of Portal can be averted.

Dockerfile Outdated
MAINTAINER Ana Balica <[email protected]>

# Updated by
LABEL HUSSAIN="[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this required, lots of people may commit on the Dockerfile in the future, we do not want to create unnecessary noise here.

Copy link
Contributor Author

@husainattar husainattar Aug 10, 2020

Choose a reason for hiding this comment

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

I actually used because in future some needs help in changing or understand the docker file.
I tried to contact ana I guess during my docker issues but didn't get a response so I thought to include my mail for any help or maintenance something. Btw I can change it right now.

FROM python:2.7
# getting the base Image
FROM python:3.6

MAINTAINER Ana Balica <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better we have an official Email ID here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes so I can remove my email.

Dockerfile Outdated
@@ -1,11 +1,33 @@
FROM python:2.7
# getting the base Image
FROM python:3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check what's the size of the resultant image? We may need to tinker with the base image to get the best results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the image size was 1.2GB with python 3.6 having 916MB. I just change the image version to keep update to date with the current documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try with 3.6-slim-buster, that is the image I use, and as far as I know that's the image with the smallest footprint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure previously actually I used 3.6-slim and 3.6-alpine but it gave me error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for that it reduced the size of the image to 600MB. I actually didn't use it first because the above-specified versions gave me an error that's why I keep as it is.

RUN apt-get update
RUN apt-get install -y software-properties-common
RUN add-apt-repository ppa:ubuntugis/ppa
RUN apt-get install -y gdal-bin python-gdal python3-gdal
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need python-gdal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because while building it gives me an error regarding gdal lib not found during executing the cities_light command. So I tried to include and it resolved error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes because while building it gives me an error regarding gdal lib not found during executing the cities_light command. So I tried to include and it resolved error.

are you sure about "python-gdal" as image using here is python3.6 , then why we need "python-gdal" library of python 2 .
As, gdal lib not found during executing the cities_light command can be fixed by only installing "python3-gdal".

Also , installing "python-gdal" here will also install python2 and other dependency in the image .

So, My suggestion is to only use "python3-gdal" here and verify for issue and also sure image size will further reduce.

web:
build: .
command: python systers_portal/manage.py runserver 0.0.0.0:8000
volumes:
- .:/usr/src/portal
- .:/usr/src/portal
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reasons for changing this?

Copy link
Contributor Author

@husainattar husainattar Aug 10, 2020

Choose a reason for hiding this comment

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

The extra space has actually no specific reason . I have just re-written the docker.yml so to maintain proper indentation I guess have added extra space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally. indentation is done in multiples of 2, could you change this back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sure I am actually not aware of indentation rule.

Fix the indentation with power of 2's 
as required.
@husainattar
Copy link
Contributor Author

Btw @SanketDG I have included some important additional commands for database and common related issues in documentation. I have just not included on how we can make changes during development and get reflect in running container instantly.

@sakshi1499 sakshi1499 removed the Status: Changes Requested Changes are required to be done by the PR author. label Aug 13, 2020
@Rajpratik71
Copy link
Contributor

@sakshi1499 ohk so I should squash all the 15 commits and create a single one commit as per guidelines?

yes squash it and make a single commit with all changes done in description

@Rajpratik71
Copy link
Contributor

Also , @husainattar to update your working branch with Upstream branch , you can do "git rebase" instead of merging it . This approach doesn't create a new commit in your branch.

@sakshi1499 sakshi1499 added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Sep 10, 2020
@sakshi1499
Copy link

@husainattar Are you working on this?

@sakshi1499 sakshi1499 added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Changes Requested Changes are required to be done by the PR author. Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants