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

Dockerfile improvements #548

Merged
merged 1 commit into from
Nov 5, 2017

Conversation

ricardogsilva
Copy link
Member

Overview

This PR improves the existing Dockerfile by adding a few missing stuff from the first implementation. Namely:

  • Addition of a --reload flag that makes the web server automatically reload whenever the code changes. This is only useful for development purposes, as it simplifies the dev workflow;
  • Usage of python's os.execlp function, which behaves similarly to bash's exec. This causes the entrypoint script to be replaced by the web server (in this case gunicorn) in such a way that the same environment and PID is used. This effectively means that gunicorn is now PID1 of the docker container, making it easier to interact with it. It also means that a running pycsw container will now exit cleanly without errors when it is stopped or removed.

Related Issue / Discussion

#530
#534

Additional Information

I have another also PR #547, which is related to this one. It adds documentation on how to use the docker image.

Contributions and Licensing

(as per https://github.com/geopython/pycsw/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to pycsw. I confirm that my contributions to pycsw will be compatible with the pycsw license guidelines at the time of contribution.
  • I have already previously agreed to the pycsw Contributions and Licensing Guidelines

@ricardogsilva ricardogsilva requested a review from kalxas November 5, 2017 17:46
@ricardogsilva ricardogsilva mentioned this pull request Nov 5, 2017
2 tasks
@ricardogsilva
Copy link
Member Author

It seems I've messed up this PR with the documentation one #547. Sorry about that.
Let me try to clean things up.

@codecov
Copy link

codecov bot commented Nov 5, 2017

Codecov Report

Merging #548 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #548   +/-   ##
=======================================
  Coverage   55.93%   55.93%           
=======================================
  Files          29       29           
  Lines        6338     6338           
  Branches     1342     1342           
=======================================
  Hits         3545     3545           
  Misses       2413     2413           
  Partials      380      380
Flag Coverage Δ
#integrationtests 54.57% <ø> (ø) ⬆️
#unittests 7.63% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c182204...a0ebb61. Read the comment docs.

@ricardogsilva ricardogsilva force-pushed the dockerfile-improvements branch from bd6996f to a0ebb61 Compare November 5, 2017 17:58
@ricardogsilva
Copy link
Member Author

OK, all good now. Please review at your convenience.

@tomkralidis
Copy link
Member

+1

@@ -8,5 +8,4 @@ pytest==3.0.3
pytest-cov==2.4.0
pytest-flake8==0.8.1
pytest-timeout==1.2.0
requests==2.10.0
Copy link
Member

Choose a reason for hiding this comment

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

is this removal related to this PR?

Copy link
Member Author

@ricardogsilva ricardogsilva Nov 5, 2017

Choose a reason for hiding this comment

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

Actually (and strangely) yes it is.

The requests dependency does not seem to be needed for pycsw - I've done some grep'ing in the codebase and I don't think we are using it anywhere (I might be wrong though).

In addition to that (and hence why it is related here), the gunicorn wsgi server, which is being used in the docker image, depends on a more recent version of requests.
When I was testing the workflow for getting a development environment based on docker (as described in the docs on the other PR) I realized that when one would do

pip install -r requirements-dev.txt

inside a running container it would uninstall the version of requests required by gunicorn and downgrade it to the one pinned in our requirements-dev.txt. This would lead to a crash in gunicorn when reloading/restarting the server's worker processes.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

@kalxas kalxas merged commit ca558a1 into geopython:master Nov 5, 2017
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