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

Post-merge Docker Compose cleanup issues #3083

Merged

Conversation

migurski
Copy link
Contributor

@migurski migurski commented Feb 4, 2021

Follow up to Tom’s review comments in #2409 (review):

  • Checkout was updated to v2 upstream, so no need to do that in this PR
  • After testing under a variety of platforms, we haven’t been able to reproduce last year’s bug so ENABLE_BOOTSTRAP is now gone.
  • Moved DB functions to /usr/local/share/ since they’re still necessary per Add Docker Compose Support for Development Environment #2409 (comment)
  • DB password and superuser privilege are both still needed when even when using trust auth because the database is needed for setup, migrations, normal usage, and data imports
  • yarnpkg install is still required since bundle exec will not work without additional app code present during Docker build step. The old option here was to leave the yarn step for the end-user after app installation, but I think it’s better more of the build process to be automated within the Dockerfile

Closes #3081

@tomhughes
Copy link
Member

Blimey I just realised what you're trying to do and why you can't use rake to do the yarn install - you're trying to cherry pick what files to map into the container!

Yes of course that won't work, but it's completely unsuppported to try and install the gems and node modules in this way.

The docker configuration needs to follow the normal installation instructions to build the image, not make up crazy stuff of it's own...

@gravitystorm
Copy link
Collaborator

* since `bundle exec` will not work without additional app code

What app code is needed? I would have expected bundle exec rake yarn:install to work after bundle is installed and bundle install has added all the gems.

@tomhughes
Copy link
Member

Well the first thing that fails is that Rakefile is missing, then if you map that in it fails because all the rails configuration files are missing.

Basically it's working in an environment where only package.json and yarn.lock have been mapped in to the container.

@tomhughes
Copy link
Member

I even wonder where it's installing the node modules to? Normally they get installed locally to node_modules in the source tree but that isn't mapped into the container at the point of the install here so are they being installed as system global modules?

@migurski
Copy link
Contributor Author

migurski commented Feb 4, 2021

Yes, that’s right: it wants the Rakefile, then it wants more. When I tried to resolve this chain, it was transforming the Dockerfile from a working install of the host’s local Ruby code for development purposes to a self-contained deployment container that wouldn't help developers make and preview changes. There might be a role for the latter but I can’t tell if it’s possible to support in the same container!

Node modules go into a non-global node_modules directory which is kept out of the host directory tree based on ~December feedback from Andy:

# Prevent these directories from mounting so they're not shared between host OS and Docker
- /app/node_modules/
- /app/tmp/

@tomhughes
Copy link
Member

I can't reproduce the bootsnap issues, either using podman on Fedora or using real docker on Ubuntu. I can remove all the BOOTSNAP_ENABLE stuff and it still works...

@migurski
Copy link
Contributor Author

migurski commented Feb 4, 2021

I just tested the old ENABLE_BOOTSNAP problem on Mac and under Github Actions, and the rake db:migrate step seems to pass just fine. I can’t tell what’s changed since it was causing grief last year!

@mmd-osm
Copy link
Contributor

mmd-osm commented Feb 4, 2021

We'll probably take the db function topic to a new issue, as @gravitystorm raised some concerns why they still would be needed. By the way, migrations did pass even without db functions in my tests earlier today.

I don't recall the ENABLE_BOOTSTRAP specifics either, but I think I experienced the same issue at one point.

@migurski migurski force-pushed the migurski/docker-compose-cleanups branch from b6ecf45 to a3e2309 Compare February 13, 2021 06:56
@migurski
Copy link
Contributor Author

I rebased this PR to keep it current.

@gravitystorm
Copy link
Collaborator

Thanks @migurski . I'm going to merge this PR since everything here looks like an improvement to me - even if the yarn:install stuff is still not ideal.

@gravitystorm gravitystorm merged commit 5761371 into openstreetmap:master Feb 17, 2021
@migurski
Copy link
Contributor Author

thanks @gravitystorm!

@migurski migurski deleted the migurski/docker-compose-cleanups branch February 17, 2021 16:16
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.

Address post-merge Docker Compose cleanup issues
4 participants