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

Add docker-compose development environment #3947

Merged
merged 1 commit into from
May 5, 2021

Conversation

waiting-for-dev
Copy link
Contributor

This allows a quick hands-on approach to contributing to Solidus:

docker-compose up -d

The ruby version in the image can be configured through the
ruby_version docker build argument (2.7.2 is used by default):

docker-compose build --build-arg ruby_version=2.6 app
docker-compose up -d

The rails version can be set when the project is booted:

RAILS_VERSION='~> 5.0' docker-compose up -d

Tests can be run setting the corresponding DB environment variable:

docker-compose exec app bin/rspec
docker-compose exec app env DB=postgres bin/rspec
docker-compose exec app env DB=mysql bin/rspec

Port 3000 is exposed to allow installing the sandbox application as
usual.

Some noticeable considerations:

  • The file database.yml in the Dummy app has been changed to
    accommodate the docker-compose setup. Taking the occasion it has been
    refactor to be more consistent. The changes should not affect any local
    setup, but it's worth that some people check their setup keeps working
    without issues.

  • The simplest configuration forces us to use root as the postgres
    user. MySQL's docker image creates the root user and grants all the
    privileges to it. Using the same user (and password) for both engines
    simplifies database.yml configuration. In case another username was to
    be chosen, MySQL wouldn't allow it to create the databases needed to do
    the testing. If we decide to change it in the future, we would need to
    provision a SQL script to the image on boot time:

services:
  mysql:
    # ...
    volumes:
      - ./docker/provision/mysql/init:/docker-entrypoint-initdb.d
-- docker/provision/mysql/init/01_databases.sql
CREATE DATABASE IF NOT EXISTS `solidus_api_test`;
CREATE DATABASE IF NOT EXISTS `solidus_backend_test`;
CREATE DATABASE IF NOT EXISTS `solidus_core_test`;
CREATE DATABASE IF NOT EXISTS `solidus_frontend_test`;

GRANT ALL ON `solidus_api_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_backend_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_core_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_frontend_test`.* TO 'solidus_user'@'%'
  • Dummy application's database.yml file no longer checks whether the
    CI environment variable is set. Instead, it's CircleCI configuration
    the one that sets DB_USERNAME to root. This is part of the work of
    making the file more consistent.

  • At first, I explored having two different images, one for each database
    engine. But I found the final solution simpler. However, it's another
    option to consider.

  • The Gemfile has been modified to allow mysql2, postgres &
    sqlite3/fast_sqlite gems to be installed at the same time. They will
    install everything in case the DB_ALL environment variable is set (as
    we are doing in the docker-compose file). This change should be backward
    compatible.

  • A new selenium capybara driver has been added to make tests pass
    inside the docker container.

Description

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Hey @waiting-for-dev, this is great. I'm not a Docker expert but this seems to be well done, thanks!

I left some preliminary questions + I think we should improve the documentation by adding some notes about Docker setup in Solidus Guides here: https://guides.solidus.io/developers/getting-started/develop-solidus.html.

backend/spec/spec_helper.rb Outdated Show resolved Hide resolved
frontend/spec/spec_helper.rb Show resolved Hide resolved
core/lib/spree/testing_support/dummy_app/database.yml Outdated Show resolved Hide resolved
@waiting-for-dev
Copy link
Contributor Author

Thanks for your review @kennyadsl .

I left some preliminary questions + I think we should improve the documentation by adding some notes about Docker setup in Solidus Guides here: https://guides.solidus.io/developers/getting-started/develop-solidus.html.

I updated the guides, thanks for pointing me to them. I have addressed the issues separately in different commits. If it's ok with you I'll squash them together when everything is ready for release.

@peterberkenbosch
Copy link
Contributor

There are some good pointers here https://evilmartians.com/chronicles/ruby-on-whales-docker-for-ruby-rails-development and https://evilmartians.com/chronicles/reusable-development-containers-with-docker-compose-and-dip that we could implement here too. Keeping the docker image small and running fast is key for adaption I think. Using volumes and the correct layering gives a lot of benefit. Having this available is awesome, thanks for that

@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Feb 22, 2021

There are some good pointers here https://evilmartians.com/chronicles/ruby-on-whales-docker-for-ruby-rails-development and https://evilmartians.com/chronicles/reusable-development-containers-with-docker-compose-and-dip that we could implement here too. Keeping the docker image small and running fast is key for adaption I think. Using volumes and the correct layering gives a lot of benefit. Having this available is awesome, thanks for that

Thank you @peterberkenbosch for sharing, that's really good stuff. I think we could adopt some of the ideas, while I'd ignore others. I'll try to go through all of them so we can all agree on what to take. Please, tell me if I overlooked something.

❌ Using dip. It's an opinionated tool that anyway can be used orthogonally with our setup (but looks really cool, I'll study adopting it myself, so thanks for letting me know about it).

✔️ Using slim image. SURE!

❌ Specifying Debian distribution. I'm not sure it's a good idea. It adds more maintenance burden for a benefit that's not too clear for me.

✔️ Installing postgres-client in the image. Yeah, makes sense.

✔️ Setting default values for build arguments from docker-compose.

✔️ Lock yarn & node versions.

✔️ Lock bundler version.

❌ Install git. I don't think most people use git from within the container. But more on this later.

✔️ Remove apt records. Makes sense.

✔️ noninteractive apt frontend. Makes sense.

✔️ Don't install apt recommendations. Makes sense.

✔️ & ❌ Using the Aptfile with dependencies. Having some dependencies declared outside the Dockerfile in this way makes sense only for a team that tries to share a Dockerfile between several projects. However, I think it's a neat trick to allow users to install what they want within the container. So, I'd add apt-get install $(cat /tmp/Aptfile 2> /dev/null | xargs) & I'd add .dockerdev/Aptfile to .gitignore. Then a user can decide to add git, vim or whatever to the image.

✔️ Use .dockerdev context in the Dockerfile. That's great!

❌ Define multiple services based on app. I thought about it but I don't think it makes sense here. I thought about adding a service with the sandbox application, but as it needs to be regenerated every time the code changes it'd be quite useless.

✔️ Tagging the image. Definitely!!!

✔️ :cached strategy & volumes to speed up Mac. Great!! I don't use Mac so I wasn't aware of these performance settings. Let's add them!!

✔️ Keep psql history. Cool!

✔️ Keep bash history. Even cooler!

❌ Setting default editor. It's user-dependent and can be configured using the Aptfile as we talked about before, and the .env file. However, I'd add .env to .gitignore.

✔️ Health checks. I didn't know about it. Makes sense!

@peterberkenbosch
Copy link
Contributor

Awesome @waiting-for-dev, this helps a lot for development. 👍

Also really like your reply and rational for your choices here. Love it.

@waiting-for-dev
Copy link
Contributor Author

@peterberkenbosch I updated the configuration following some of the tips from the article. Some changes from what I said in the previous comment:

  • Of course, I had to add MySQL into the equation.
  • I did have to specify the Debian version in order to add APT sources for some manually installed packages.
  • No need to use yarn.
  • I had to install git so that Gemfile sources can be fetched from Github.
  • Limited the clean-up of apt sources to rm -rf /var/cache/apt/lists/*. It's what docker recommends and I want to keep things simple.
  • Not using the Aptfile trick. Docker COPY command doesn't allow for a file not to exist. There are workarounds but again I want to keep things simple, at least for now. Anyway, it's not a standardized workflow with Docker, so it would be far from evident.
  • No healthy checks for DB services. docker-compose no longer supports condition. There are workarounds but I don't think adding extra complexity pays off here. So I only switched links for depends_on, which will at least ensure the startup order.

Please, look at 737464d

Copy link
Contributor

@blocknotes blocknotes left a comment

Choose a reason for hiding this comment

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

Nice work @waiting-for-dev!
I just added some minor considerations but looks good to me.

.dockerdev/Dockerfile Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
POSTGRES_USER: root
POSTGRES_PASSWORD: password
volumes:
- postgres:/var/lib/postgresql/data
Copy link
Contributor

Choose a reason for hiding this comment

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

A curiosity here: is there a special reason to have a named volume for DB data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's so that we can keep the data in case we remove the container.

docker-compose.yml Show resolved Hide resolved
.dockerdev/Dockerfile Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@waiting-for-dev
Copy link
Contributor Author

In 591f6cc I have adapted the database.yml for the dummy application that is generated from extensions to be also configurable from docker (and consistent with the one within spec directory).

I have removed the reference to Travis as I think we're not using it anymore. I also have removed a duplicated configuration for sqlite. postgres is no longer the default username for Postgres, but as it's already the default in PostgresSQL itself it should be a safe change.

As @kennyadsl said, cleaning and unifying how the database.yml files are generated should be part of another PR.

waiting-for-dev added a commit to solidusio/solidus_starter_frontend that referenced this pull request Mar 1, 2021
See solidusio/solidus#3947 for details on the decisions that have been
taken.

We are also decoupling the source of solidus & solidus_i18n
dependencies, and providing more flexibility by allowing the
specification of both repository & branch for each project (therefore
allowing working with forks).
@kennyadsl kennyadsl added this to the 3.0.0 milestone Mar 2, 2021
@waiting-for-dev
Copy link
Contributor Author

I added a new commit that allows configuring the exposed port (defaults to 3000):

SANDBOX_PORT=4000 docker-compose up -d
docker-compose exec app rails server --binding 0.0.0.0 --port 4000

@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Mar 3, 2021

I added a new commit that generates the sandbox application taking into account what is configured as host/username/password for the given database engine.

waiting-for-dev added a commit to solidusio/solidus_starter_frontend that referenced this pull request Mar 23, 2021
See solidusio/solidus#3947 for details on the decisions that have been
taken.

We are also decoupling the source of solidus & solidus_i18n
dependencies, and providing more flexibility by allowing the
specification of both repository & branch for each project (therefore
allowing working with forks).
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I'm not a docker user but this seems legit if we want to be more friendly with who does. Thanks, @waiting-for-dev!

This allows a quick hands-on approach to contributing to Solidus:

```bash
docker-compose up -d
```

The ruby version in the image can be configured through the
`ruby_version` docker build argument (at this point, 2.7.2 is used by
default):

```bash
docker-compose build --build-arg ruby_version=2.6 app
docker-compose up -d
```

The rails version can be set when the project is booted up:

```bash
RAILS_VERSION='~> 5.0' docker-compose up -d
```

Tests can be run setting the corresponding `DB` environment variable:

```bash
docker-compose exec app bin/rspec
docker-compose exec app env DB=postgres bin/rspec
docker-compose exec app env DB=mysql bin/rspec
```

By default, port `3000` is exposed to allow installing the sandbox
application as usual. However, it can be configured through the
`SANDBOX_PORT` environment variable:

```bash
SANDBOX_PORT=4000 docker-compose up -d
docker-compose exec app bin/sandbox
docker-compose exec app bin/rails server --binding 0.0.0.0 --port 4000
```

Some considerations:

- The file `database.yml` in the Dummy app has been changed to
accommodate the docker-compose setup. Taking the occasion it has been
refactor to be more consistent.

- The simplest configuration forces us to use `root` as the postgres
user. MySQL's docker image creates the `root` user and grants all the
privileges to it. Using the same user (and password) for both engines
simplifies `database.yml` configuration. In case another username was to
be chosen, MySQL wouldn't allow it to create the databases needed to do
the testing. If we decide to change it in the future, we'll need to
provision a SQL script to the image on boot time:

```
services:
  mysql:
    # ...
    volumes:
      - ./docker/provision/mysql/init:/docker-entrypoint-initdb.d
```

```sql
-- docker/provision/mysql/init/01_databases.sql
CREATE DATABASE IF NOT EXISTS `solidus_api_test`;
CREATE DATABASE IF NOT EXISTS `solidus_backend_test`;
CREATE DATABASE IF NOT EXISTS `solidus_core_test`;
CREATE DATABASE IF NOT EXISTS `solidus_frontend_test`;

GRANT ALL ON `solidus_api_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_backend_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_core_test`.* TO 'solidus_user'@'%';
GRANT ALL ON `solidus_frontend_test`.* TO 'solidus_user'@'%'
```

- Dummy application's `database.yml` file no longer checks whether the
`CI` environment variable is set. Instead, it's CircleCI configuration
the one that sets `DB_USERNAME` to `root`. This is part of the work of
making the file more consistent.

- MySQL and Postgres hosts are now configurable through `DB_MYSQL_HOST` &
  `DB_POSTGRES_HOST`. This allows us link to both services from the
  `app` container.

- At first, I explored having two different images, one for each database
engine. But I find the final solution simpler. However, it's another
option to consider.

- The `Gemfile` has been modified to allow mysql2, postgres &
sqlite3/fast_sqlite gems to be installed at the same time. They will
install everything in case the `DB_ALL` environment variable is set (as
we are doing in the docker-compose file). This change should be backward
compatible.

- A new selenium capybara driver has been added to make tests pass
inside the docker container.
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Apr 19, 2021

I'm not a docker user but this seems legit if we want to be more friendly with who does. Thanks, @waiting-for-dev!

Thanks @kennyadsl . I rebased from master and I added a new line to the Dockerfile:

ENV PATH=$PATH:$GEM_HOME/bin

It adds gem executables to the PATH so that we can, for instance, run rails.

waiting-for-dev added a commit to solidusio/solidus_starter_frontend that referenced this pull request Apr 19, 2021
See solidusio/solidus#3947 for details on the decisions that have been
taken.

We are also decoupling the source of solidus & solidus_i18n
dependencies, and providing more flexibility by allowing the
specification of both repository & branch for each project (therefore
allowing working with forks).
@kennyadsl kennyadsl modified the milestones: 3.0.0, 3.1.0 Apr 21, 2021
@kennyadsl kennyadsl merged commit d4925d8 into solidusio:master May 5, 2021
@kennyadsl kennyadsl deleted the docker branch May 5, 2021 09:19
waiting-for-dev added a commit to solidusio/solidus_starter_frontend that referenced this pull request May 6, 2021
See solidusio/solidus#3947 for details on the decisions that have been
taken.

We are also decoupling the source of solidus & solidus_i18n
dependencies, and providing more flexibility by allowing the
specification of both repository & branch for each project (therefore
allowing working with forks).
waiting-for-dev added a commit to solidusio/solidus-demo that referenced this pull request Aug 13, 2021
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Jun 8, 2022
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.

5 participants