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

Try not to mount app directory only /public/system #4858

Closed
wants to merge 12 commits into from

Conversation

icarito
Copy link
Member

@icarito icarito commented Feb 20, 2019

Intends to fix #3261 and also possibly #4849 and other issues arising from mountint the app dir as a volume in container.

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • PR is descriptively titled 📑
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@plotsbot
Copy link
Collaborator

plotsbot commented Feb 20, 2019

2 Messages
📖 @icarito Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progress’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@icarito
Copy link
Member Author

icarito commented Feb 25, 2019

This is passing now! I'll test it out in unstable, @jywarren please review if https://github.com/icarito/plots2/blob/not_mount_app_volume/containers/docker-compose-production.yml#L40-L45 sounds like all directories needed at runtime for writing.

@icarito icarito added the testing issues are usually for adding unit tests, integration tests or any other tests for a feature label Feb 25, 2019
@icarito icarito changed the title [WIP] Try not to mount app directory only /public/system Try not to mount app directory only /public/system Feb 25, 2019
@icarito icarito changed the title Try not to mount app directory only /public/system [WIP] Try not to mount app directory only /public/system Feb 25, 2019
@icarito
Copy link
Member Author

icarito commented Feb 25, 2019

Tried this in unstable and found 404 for public/system files, investigating...

@icarito icarito changed the title [WIP] Try not to mount app directory only /public/system Try not to mount app directory only /public/system Feb 25, 2019
@icarito
Copy link
Member Author

icarito commented Feb 25, 2019

Issue was with assets precompilation. This is running in unstable now please try it out!

@grvsachdeva
Copy link
Member

Hi @icarito, please update the status of the issue. Thanks!

@@ -38,3 +38,4 @@ WORKDIR /app

RUN yarn install && yarn upgrade
RUN passenger-config compile-nginx-engine --connect-timeout 60 --idle-timeout 60
RUN rake assets:precompile && rake tmp:cache:clear
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main insight to put assets inside the Docker container. This way we can push to production without running bundler / yarn in production.

@icarito
Copy link
Member Author

icarito commented Apr 9, 2019

Hi! I believe this is ready for merging - it does change fundamentally how we treat assets and in particular how we access local files (specific needed directories only are "mounted") - all else will be inside the container.
We should test this out in unstable - to confirm stuff like uploading files works in all instances.
This is a step in the direction of deploying pre-built containers to production instead of building them on the production server. Also it should fix issues with Gemfile.lock and Yarn.lock when deploying, that @jywarren reported.

@icarito
Copy link
Member Author

icarito commented Apr 9, 2019

Except travis just failed on it, checking again!

@icarito
Copy link
Member Author

icarito commented Apr 9, 2019

Wow it looks like since I made this, we upgraded to a version of rails that doesn't use initialize_on_precompile=false which is what I was doing to be able to compile assets without a database.
I need to find a workaround.

@icarito
Copy link
Member Author

icarito commented Apr 9, 2019

There is a workaround for this detailed here:

passing a dummy database config while precompiling:

bundle exec rake RAILS_ENV=staging DATABASE_URL=mysql2://none:[email protected]/none assets:precompile

I think we need #5422 in order to have a workaround for this. So this is stuck for the moment.

@jywarren jywarren changed the base branch from master to main June 30, 2020 17:47
@stale
Copy link

stale bot commented Oct 7, 2020

Hi 😄, this issue has been automatically marked as stale because it has not had recent activity. Don't worry you can continue to work on this and ask @publiclab/reviewers to add "work in progress" label 🎉 . Otherwise, it will be closed if no further activity occurs in 5 days -- but you can always re-open it if you like! 💯 Thank you for your contributions 🙌 🎈.

@stale stale bot added the stale label Oct 7, 2020
@stale stale bot closed this Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress stale testing issues are usually for adding unit tests, integration tests or any other tests for a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid running bower twice for testing
3 participants