-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix docker environment #34
base: master
Are you sure you want to change the base?
Conversation
Ideally this could be configured with environment variables, but in the meantime we're going to have to hard code it to where it is in Docker. In other environments, it can be updated.
This is where the data ends up when you run the command in the readme. This feels very fragile though -- it really needs to scripts to build this data.
1744c36
to
70c621b
Compare
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.
Thanks for the changes. I'm going to give you comment on the specific items. (this comment actually was accidental abut I cannot remove it anymore)
Ah just seen #27. Updating the config in the build seems sensible, that way it'll still work for localhost environments. I think an environment variable would be the best solution though -- Docker sings when things can be configured with environment variables. |
What about passing the variable when running the build? and we do a replacement in the config.yml in the docker build script? |
Yep, that works. Environment variables work much better because you can update them at runtime, but putting it in the build works for the time being. |
We could update this by passing the configuration file directly as of with the latest docker version |
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.
Those are the same changes that I did in my local branch, to me are good.
The problem with this changes is that we are hard-coding stuff in the local version, for the docker-compose which is just one way to run the system. The cleanest solution, as I wrote before, is to decouple the local configuration from the docker-compose configuration and pass the modified configuration file directly, as It's done here: https://github.com/kermitt2/biblio-glutton/blob/feature/docker/docker-compose.yml#L10 The only disadvantage of such approach is to remember to keep in sync the local and compose configuration file. |
A few things that didn't work when I switched to upstream. Details in commit messages.
It really needs some scripts to load the data, though. It'll be pretty much impossible for somebody new to this to get it to work without scripts, because juggling paths inside and outside of the container is very complicated now there is a two-step build. (The paths inside the container are completely different to those outside!)
To me, this is an argument to get rid of the two-step build process. The complexity is just not worth the disk space saved.