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 a Dockerfile to ease development #66

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

Conversation

gkokolatos
Copy link
Contributor

Additionally document in code the required setup and have an example of use. It
is by no means intended to be run in production.

The base image is from ubuntu 18.04 LTS which is the most recent version of
ubuntu that can match the python/pip/virtualenv requirements. For example ubuntu
20.04 will require Pillow 6.2 instead of 5.4.1 and virtualenv will have to be
launched without the -no-site-packages option.

The setup is based on the name pgeusystem which is used throughout, e.g. admin
name, db name etc.

Some basic instructions on how to build, run and use the image are added to the
README on the devsetup directory, which by itself is now listed in the root
README file to provide visibility.

Additionally document in code the required setup and have an example of use. It
is by no means intended to be run in production.

The base image is from ubuntu 18.04 LTS which is the most recent version of
ubuntu that can match the python/pip/virtualenv requirements. For example ubuntu
20.04 will require Pillow 6.2 instead of 5.4.1 and virtualenv will have to be
launched without the -no-site-packages option.

The setup is based on the name pgeusystem which is used throughout, e.g. admin
name, db name etc.

Some basic instructions on how to build, run and use the image are added to the
README on the devsetup directory, which by itself is now listed in the root
README file to provide visibility.
@mhagander
Copy link
Member

The biggest deployments of the system, being the PGEU and PGUS systems, are all running on top of Debian Buster rather than Ubuntu. I wonder if it might be a better idea to base the dockerfile off that?

In those deployments we also drive the majority of the packages off the DEB packages rather than manual installations. That's where most of the version definitions in the original requirements come from.

As for the usecase -- I think it'd be good to at least have an option where it runs on top of a git checkout and uses the code there, rather than deploys the app itself into the container. That is, mount the git root from the host into the container and then run it. As I understand it from the suggested Dockerfile, it requires me to rebuild the container every time I change a file -- I'd like it to just run off the file as it is outside the container in many cases.

@gkokolatos
Copy link
Contributor Author

gkokolatos commented Mar 18, 2021 via email

* Now the base image is Debian Buster.
* Packages are installed from debs, not manually
* The build stage does not verify for correctness, it marely prepares packages
  and the system.
* There exists the ability to mount the volume in the container, as it is not
  copied
* There exists the ability to run with the mounted volume, and to not need to
  rebuild for minor changes
@andreasscherbaum
Copy link
Member

I agree that it might be a better choice to run this on debian.

Related ticket: #65 (for the virtualenv deprecated option)

@gkokolatos
Copy link
Contributor Author

gkokolatos commented Mar 18, 2021 via email

@gkokolatos
Copy link
Contributor Author

@mhagander, @andreasscherbaum

Any comments on the updated version?

set -e\n\
pg_ctl -D /opt/pgeusystem/pgdata -l /opt/pgeusystem/pgdata/logfile start\n\
pushd /opt/pgeusystem/app/\n\
./tools/devsetup/dev_setup.sh localhost 5432 pgeusystem pgeusystem\n\
Copy link
Member

Choose a reason for hiding this comment

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

My inner monk is confused by the missing spaces 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.

I am confused by your confusion. Sorry, which missing spaces are you referring to?

Copy link
Member

Choose a reason for hiding this comment

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

The spaces before "./tools", which are fewer than the other ones. But that's just eye candy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will fix :)

If the above commands are successfull then one can reach the index page of the
app in http://localhost:8012

If the user so wishes, can reach the database in the running container via:
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean you have to login into the container first?
Then it would be useful to add the command how to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest above to run the image with the --network host parameter. When that is done, and is successful, then it is not necessary to login into the container first.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, perfect.

@andreasscherbaum
Copy link
Member

Haven't tested the image (have a local version running). Left two comments, otherwise this looks good.

@gkokolatos
Copy link
Contributor Author

Haven't tested the image (have a local version running). Left two comments, otherwise this looks good.

Thank you for looking!

* Fix whitespace missmatch during .sh content generation
@mhagander
Copy link
Member

Is just me not reading it right, or does this re-run the devsetup script every item the container is started? Including creating a new virtualenv every time?

First, doesn't that seem like something that should be run just once? Otherwise you have to do things like create the superuser over and over again?

Second, why create a virtualenv inside the docker container. That seems like doubling things?

@rjuju
Copy link

rjuju commented Mar 31, 2021

Also, why not exposing the postgres and django ports rather than using host network? It would allow users to let them map those to a local port if they wish to.

@gkokolatos
Copy link
Contributor Author

gkokolatos commented Mar 31, 2021 via email

@gkokolatos
Copy link
Contributor Author

gkokolatos commented Mar 31, 2021 via email

@rjuju
Copy link

rjuju commented Mar 31, 2021

On Wednesday, March 31, 2021 6:34 PM, Julien Rouhaud @.***> wrote: Also, why not exposing the postgres and django ports rather than using host network? It would allow users to let them map those to a local port if they wish to.
It is by no means necessary, it is an example of one of the many many available build options. I find it to be the most straight forward. Users that do know and use docker will choose their own. Users that are not so comfortable with it, they will not get confused by all the additional arguments in the example. Or that is what I thought.

I'm talking about a simple EXPOSE directive, to make it clear what ports are used. If I'm not mistaken using --network host will silently ignore already ports that are already used on the host , so I'm worried that in many case people trying to run this image will connect to their local postgres instance rather than the container one.

@mhagander
Copy link
Member

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, March 31, 2021 4:56 PM, Magnus Hagander @.***> wrote: Is just me not reading it right, or does this re-run the devsetup script every item the container is started? Including creating a new virtualenv every time?
Only if you choose to, and yes otherwise which is also the default.

I'm not entirely sure what you mean here. But supporting both methods are definitely good -- but we should probably mention them in the README in that case?

First, doesn't that seem like something that should be run just once? Otherwise you have to do things like create the superuser over and over again? Second, why create a virtualenv inside the docker container. That seems like doubling things?
I understand your argument and is valid. Allow me to be clear on the intention though, which is documentation as code. The first time I checkout this codebase I spend hours trying to make it run on my out of the box ubuntu 20.04 machine. Finding out which packages to install, which to downgrade, which to update along with all the other assumptions made. This is nothing new to any codebase for sure, yet it can be a bit better. The only thing this dockerfile is trying to do, is to show which OS is favoured, in which version, with which packages installed and what steps to take in order to run it. If what I would use to run locally does create a virtualenv, then so be it. Of course there are many other ways of documenting infrastructure, this is simply a low hanging fruit. A next step if this or a version of it gets in, would be to create a minimal travis script that will also make certain that the dockerfile manages to run.

I definitely agree with this in principle.

But my comment is, you are now installing things twice. Once in the container and then again in the virtualenv inside said container.

I think it should either do system for what it can and virtualenv for what's needed (that's what we do in the pgeu deployments, and the only things that actually go int he virtualenv are django itself and qrencode, (and django-markdown django-markwhat, but I dont't hnk those are needed anymore so we should remove them rom requirements.txt). Or we should do everything from virtualenv and not do anything as apt packages.

The first option (keeping most in system packages) would certainly be the easiest no?

@gkokolatos
Copy link
Contributor Author

gkokolatos commented Apr 22, 2021 via email

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.

4 participants