-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Docker-compose dev environment #1133
Conversation
conf/openlibrary-docker.yml
Outdated
editions_view: http://127.0.0.1:5984/editions/_fti/_design/seeds/by_seed | ||
|
||
admin: | ||
olsystem_root: /home/noufal/projects/OL/olsystem/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be pruned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in code here:
openlibrary/openlibrary/plugins/admin/code.py
Line 538 in 75452cf
f = open("%s/olsystem.yml"%config.admin.olsystem_root) |
/admin/services
to display. Looks like it is not registered in the setup()
further down in that file. Shall I prune the entire service_status
class?
conf/openlibrary-docker.yml
Outdated
|
||
libraries_admin_email: admin@dev-instance | ||
|
||
graphite_base_url: http://ol-admin.us.archive.org:7080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want developer instances recording to graphite (analytics)
conf/openlibrary-docker.yml
Outdated
libraries_admin_email: admin@dev-instance | ||
|
||
graphite_base_url: http://ol-admin.us.archive.org:7080 | ||
geoip_database: var/cache/GeoLiteCity.dat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mekarpeles what is this for, I don't see it in olsystem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used in https://github.com/internetarchive/openlibrary/blob/75452cf6f0e3c23e1030395c93e232686ebaa417/openlibrary/core/geo_ip.py , but there is no GeoLiteCity.dat at any of the locations
lgmt, requesting one more pair of eyeballs; there's a lot here. Config especially looks much better |
Thanks @mekarpeles ! @cdrini, could you please do a quick review and let me know if you see anything that is alarming, or could do with improvement? I'd like to get these change in soon as I have another (import API, not docker) PR that is enabled by these changes :) |
👍 I can take a look/test it out now! These changes look great, btw :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few small changes. I wouldn't close #1064 yet since I want to discuss the matter a little more (:P), but I'll remove it as a subtask of removing vagrant (assuming no resistance). I do think the split is much clearer as a result of this PR. :)
docker/README.md
Outdated
@@ -33,8 +33,7 @@ docker-compose stop | |||
docker-compose rm -v | |||
``` | |||
|
|||
Note: You must build `olbase` first before `oldev`. Currently (August 2018) the division is a bit arbitrary. More should be moved into `olbase` once we clarify | |||
the production deployment requirements. Currently these docker images are only intented for development environments. | |||
Note: You must build `olbase` first before `oldev`. `olbase` is intended to be the core Open Library image, acting as a base for production and development. `oldev` adds a test databse and any other tools that are helpful for local development. Currently (Sep 2018) these docker images are only intented for development environments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: databse
docker/ol-docker-start.sh
Outdated
--state-file solr-update.offset \ | ||
--ol-url http://web/" & | ||
|
||
# ol server, running in the foreground to avoid exiting container | ||
su openlibrary -c "authbind --deep scripts/openlibrary-server conf/openlibrary.yml --gunicorn -w4 -t180 -b:80" | ||
su openlibrary -c "authbind --deep scripts/openlibrary-server $CONFIG --gunicorn --reload -w4 -t180 -b:80" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you place the parameters on separate lines, like the preceding command? It would make it a little easier to read. Also replace those short parameter names with the full-length ones.
I'm not sure if it's because of this PR, but #1051 also no longer appears to be an issue \o/ |
@cdrini if this fixes #1051, that is great! Keep an eye on it, because even though I think I see how this might have fixed it (some of those dirs will no longer be accessing the windows fs, but using the docker volume mounts) I think some of the symlinks will still be mounted from Windows. Test that running |
@cdrini I think I have addressed all of your comments! Last one was the comments explaining the volume and bind mounts 👍 🙇♂️ Let me know if its good to go! |
w/ latest changes, this lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, found one more typo :P everything else looks good though!
@@ -1,55 +1,32 @@ | |||
# Open Library fastcgi configuration | |||
# Open Library Docker (dev) configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this should say vagrant? :P
Closes #1063
makes some changes related to ;) #1064
and hopefully #1065
Description:
Clarifies the split between
olbase
andoldev
olbase
is a single application standard image that will run OL as theopenlibrary
user, needs to be deployed sensibly to be usefuloldev
has whatever else we need to operate our dev environment, i.e. pre-populated dev db, other services and tools.oldev
may, and should, be reduced over time as we split out services.Volume mounts local dev directory and creates docker compose volumes to retain the js + css that was created at build time, and the
vendor
dir that is used to fetch git submodules, which is also populated on build.Uses a new
openlibray-docker.yml
config file to not interfere with vagrant, and avoids usingsed
to fix things. I'd suggested we rename this back toopenlibrary.yml
when we fully remove all traces of vagrant.Adds the gunicorn
--reload
option in dev so code changes are automatically detected and reloaded 🎉Usage
With the volume mount in place and the gunicorn
--reload
option a developer can edit code locally and the webserver will automatically reload the code on save.The instructions in docker/README.md are 👌