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

Docker improvements #28

Merged
merged 4 commits into from
Mar 12, 2018
Merged

Conversation

Firefishy
Copy link
Contributor

@Firefishy Firefishy commented Mar 8, 2018

Update Dockerfile and remove hardcoded SECRET_KEY_BASE

No breaking changes with current iridakos/duckrails container.

  • Update Dockerfile to use official ruby image instead of rvm
  • Use local repo instead of git cloning
  • Add .dockerignore to ignore files not of value in container
  • Add docker-entrypoint.sh to check SECRET_KEY_BASE and set if needed. Fix: Docker container should not ship with pre-defined SECRET_KEY_BASE #27
  • Add puma config with preload_app
  • Update ruby 2.4.2 -> 2.4.3

@Firefishy Firefishy changed the title Docker improvements + #27 fix Docker improvements Mar 8, 2018
@Firefishy Firefishy force-pushed the docker-improvements branch 2 times, most recently from 08f9f57 to de50a62 Compare March 8, 2018 23:40
No breaking changes with current iridakos/duckrails container.

* Update Dockerfile to use official ruby image instead of rvm
* Use local repo instead of git cloning
* Add .dockerignore to ignore files not of value in container
* Add docker-entrypoint.sh to check SECRET_KEY_BASE and set if needed
* Add puma config with preload_app
@Firefishy Firefishy force-pushed the docker-improvements branch from de50a62 to 2ff63a0 Compare March 9, 2018 11:11
@iridakos
Copy link
Owner

iridakos commented Mar 9, 2018

Great work @Firefishy! I will check it and merge it a.s.a.p.

Thank you very much for the contribution 👍

@Firefishy
Copy link
Contributor Author

I have a quick fix on the pg gem coming soon. rails/rails#31673

@Firefishy
Copy link
Contributor Author

Added the correct gem versions for pg and mysql2 rails compatibility.

@Firefishy
Copy link
Contributor Author

Firefishy commented Mar 9, 2018

I have another PR for adding ENV support todatabase.yml (while keeping sqlite3 compatibility if unset ) and an example docker-compose.yml with postgres support. I can roll it in here or keep it as a separate PR, whichever you prefer?

@iridakos
Copy link
Owner

iridakos commented Mar 9, 2018

Whatever you prefer!

By the way, I haven't looked at the changes yet.

My only concern is not to make the process of starting the app hard for the users since it is a development tool. I wouldn't like to enforce them setting env variables but give it as an option.

I will come back to you in a couple of days. Thanks for your help!

@Firefishy
Copy link
Contributor Author

I'll keep further changes for another PR. This PR is already big enough. ;-)

@iridakos iridakos merged commit 24d3c6c into iridakos:master Mar 12, 2018
@iridakos
Copy link
Owner

@Firefishy excellent work! Thank you very much for the contribution!

Note:
I removed mysql & pg gem dependencies. As stated in the README file, if someone wants to use these databases, the entries have to be manually added to the Gemfile. Users that need to run the app natively don't need to have pg or mysql related packages installed on their systems if they don't want to use them.

@Firefishy
Copy link
Contributor Author

But it means if someone wants to use a postgres database they'll need to manually patch the Gemfile and manually rebuild the docker image. A universal Docker image makes much more sense to me.

@iridakos
Copy link
Owner

If someone wants to run the application natively, he/she will have to install files on their system for these gems to install properly and that's something I don't want to happen. DuckRails is a development tool and in most (if not all) cases sqlite3 is more than enough.

I would prefer copying a new Gemfile (ex Gemfile.docker) including the adapters you suggest inside the docker image and moving on with that.

@Firefishy
Copy link
Contributor Author

sqlite3 locks-up the entire app when concurrent developers use duckrails, especially used using high-concurrency automated tests.

Would you consider just the pg gem in Gemfile then? Most developers have at least the pg libs installed.

@iridakos
Copy link
Owner

I removed the gemfile entries because I know both me and my colleagues don't have the pg libs installed.

Do you have any objections on using another Gemfile for the docker image since you need to support pg there? It would produce your desired behaviour.

sqlite3 locks-up the entire app when concurrent developers use duckrails, especially used using high-concurrency automated tests.

sqlite3 allows concurrent reads and has improved the writing locking mechanism as well. Writing in DuckRails takes place when creating/updating mocks. I can't see a real life case with tests doing that unless I'm missing something.

@Firefishy
Copy link
Contributor Author

Firefishy commented Mar 12, 2018

Yes, using another Gemfile then makes it a lower class citizen. Your existing docs detail adding pg, but are wrong because the latest un-versioned pg isn't supported by Rails 4.

sqlite3 does lock in Rails under load. Likely due to Rails using Transactions, not-passing the correct locking state level per query or similar. I'll produce some real metrics as we've seen frequent deadlocks under load. Migrating an existing instance from sqlite3 to postgres isn't trivial.

I can add docs about running:
sudo apt-get install libpq-dev
yum install postgresql-devel
brew install postgresql

@iridakos
Copy link
Owner

In my mind, we have to decide whether we will demand from users to install an external dependency that they won't even use (since DuckRails defaults to sqlite3) or let them go with the defaults and customize their DuckRails setup with manual installation and configuration of another database (like postrgresql) if they need to.

I totally understand your point but I am not convinced that this dependency should be introduced as a default for the tool. I've been using DuckRails heavily and I don't believe that the scenario in which sqlite3 locks under Rails is a usual case. I haven't noticed anything like that.

Your existing docs detail adding pg, but are wrong because the latest un-versioned pg isn't supported by Rails 4.

Are you referring to this?

Important note: If you change the database adapter, make sure you include the appropriate gem in your Gemfile (ex. for mysql gem 'mysql2') and "rebundle" to fetch & install the new gem.

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.

2 participants