Closed FlyingUnicornSF closed 5 years ago
@jayrbolton Would you review this also? Thank you!
Looks good. Two suggested changes that don't need to happen now, but can be added in the future:
python:3.7-alpine
is 30mb and python:3.7-slim
is 50mb -- i think it's worth switching to one of thoseTested locally and appears to work, so unless there are objections I'll merge and test on CI later today.
- kbase/kb_python is a 1GB image.
python:3.7-alpine
is 30mb andpython:3.7-slim
is 50mb -- i think it's worth switching to one of those
Yeah, there's nothing in the code that needs the KBase python image. But it's a low priority.
Looks good. Two suggested changes that don't need to happen now, but can be added in the future:
- kbase/kb_python is a 1GB image.
python:3.7-alpine
is 30mb andpython:3.7-slim
is 50mb -- i think it's worth switching to one of those- At some point in the future, install gevent and set the number of workers. We could use a script similar to this: https://github.com/jayrbolton/py_microapi_template/blob/master/scripts/start_server.sh
For a core service, our standard is to use our fork of the dockerize entrypoint program: https://github.com/kbase/dockerize
It is setup so that configuration comes in via environment variables in a standard way, and we can use git for these configurations. You can just pull the appropriate binary out of that repo when building your docker image, and set it up as the entrypoint. I can walk you through it when you're ready. But this should be done before it goes to prod.
@sychan I'm wondering if it's necessary to use dockerize in this case. I'm looking at the three features of dockerize, and not finding them applicable here (she has no config files, only env vars; she has no log files; the main process does not need to wait on any other services to boot). This is the setup I had for the relation engine api and it seemed to work okay: https://github.com/kbase/relation_engine_api/blob/master/Dockerfile
Its a legitimate question, and I agree that it isn't strictly necessary. But there is another case that dockerize covers, which is that it can pull environment variables from a remote URL (like a git repo). I think it is useful as a training exercise, and as a matter of release hygiene to get it all uniform.
FWIW, I haven't bugged you about the relation engine entrypoint because it is still late beta, but you should really be using dockerize as well. Getting the release pipeline as uniform as possible makes things easier in the long term.
@jayrbolton See above
This mounts the application in a WSGI application: app.config['APPLICATION_ROOT'] = os.environ.get('ROOT_PREFIX', '/applist') app.wsgi_app = DispatcherMiddleware(app.wsgi_app, { app.config['APPLICATION_ROOT']: app })