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

Use Alpine Image for Docker #12905

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

macanudo527
Copy link
Collaborator

@macanudo527 macanudo527 commented Oct 11, 2024

What? Why?

This replaces the Ubuntu image with a more lightweight image optimized for Ruby, which reduces the size of the images on the computer and simplifies the build. It also eliminates the need to prepend commands with bundle exec and the need for any version manager in the image.

I also added OFN_REDIS_TEST_URL so that tests using SpreePreferences now pass.

Some disadvantages:
When the ruby version is upgraded, the image will need to be updated and rebuilt.
Since Open Food Network uses specific versions of default gems that might not match the Alpine image's default gems, adjustments will need to be made. Currently, OFN's BigDecimal needed to be upgraded to 3.1.1 and the Alpine image's rake and bundler needed to be upgraded to match OFN's.

What should we test?

Build and spin up the docker container:

docker/build

docker/server

Verify that there are no fatal errors.
Visit HTTP://localhost:3000/

This has been tested under Ubuntu, Windows w/WSL, and Windows, but not on Mac

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@rioug
Copy link
Collaborator

rioug commented Oct 14, 2024

@macanudo527 there is a failed spec rspec ./spec/lib/reports/sales_tax_totals_by_order_spec.rb:141 that seems related to the BigDecimal upgrade, could you have a look at it ?

@macanudo527
Copy link
Collaborator Author

The last error is a bit strange. It is saving the unit_presentation wrong:

 Spree::Variant Create (1.0ms)  INSERT INTO "spree_variants" ("weight", "product_id", "cost_currency", "unit_value", "display_as", 
"variant_unit", "unit_presentation", "created_at", "updated_at", "shipping_category_id", "primary_taxon_id", "supplier_id") VALUES
 ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING "id"  [["weight", "0.0"], ["product_id", 12], ["cost_currency", "AUD"], 
["unit_value", 0.5001], ["display_as", ""], ["variant_unit", "volume"], ["unit_presentation", "500.09mL"], ["created_at", "2024-10-15 
23:48:04.298869"], ["updated_at", "2024-10-15 23:48:04.298869"], ["shipping_category_id", 1], ["primary_taxon_id", 5], 
["supplier_id", 3]]
08:48:04 rails.1   |   ↳ app/models/spree/product.rb:261:in `ensure_standard_variant'

@rioug
Copy link
Collaborator

rioug commented Oct 16, 2024

@macanudo527 the last Product Refactor PR has been merged , you'll probably need to rebase your branch

@macanudo527
Copy link
Collaborator Author

@sigmundpetersen Did you make an accidental update?

@sigmundpetersen
Copy link
Contributor

Hi @macanudo527 I just rebased your branch.
The build error may be due to the recent merge of #12787 .
I can also try rebuild a few times to see if it's a flaky error

@macanudo527
Copy link
Collaborator Author

Hi @macanudo527 I just rebased your branch.
The build error may be due to the recent merge of #12787 .
I can also try rebuild a few times to see if it's a flaky error

OK, that's kind of strange the compare looked like it was strangely incomplete, but maybe the tests are just flakey. Thanks!

@sigmundpetersen
Copy link
Contributor

Rebuild was green so I'm moving this to Code Review again 👍

Copy link
Member

@mkllnk mkllnk 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!

I'm not quite sure on the way forward though.

Dockerfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
@macanudo527 macanudo527 force-pushed the docker/use_alpine_image branch 3 times, most recently from ba3fe27 to 7b03d15 Compare November 7, 2024 15:11
@macanudo527
Copy link
Collaborator Author

@mkllnk I revised it so that you can optionally build it with the Alpine image by adding it to the build script.
There is a significant difference in build time and size:
Alpine:
2m6.737s
Ubuntu:
7m52.583s

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks gone now ! But I think we should wait for #12950 before merging this has there are still some BigDecimal related changes in this PR.

If you are keen to play with Docker, it would be nice to dockerise OFN, ie have a separate Image for the OFN code, and each dependency (Postgres, Redis etc...) It would be a good start to moving toward a Docker based deployment, especially now that Kamal exists

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

I took a look so I could keep up to date with what's happening. I have a few queries.

FROM ruby:3.1.4-alpine3.19 AS base
ENV LANG=C.UTF-8 \
LC_ALL=C.UTF-8 \
TZ=Europe/London \
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it's possible to inherit the timezone from the host? That seems like the most intuitive for the developer.
(Also note that London is not always the same as GMT/UTC due to daylight savings)

* To run tests or access the server directly, you can use bash:
```sh
$ docker compose exec web bash
```
Copy link
Member

Choose a reason for hiding this comment

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

These commands look much the same between Linux and Windows, we should consider combining the sections in the future.

@@ -39,14 +39,34 @@ $ git clone [email protected]:openfoodfoundation/openfoodnetwork.git
```sh
$ cd openfoodnetwork
```
* Download the Docker images, build the Docker containers, seed the database with sample data, AND log the screen output from these tasks:
* You have two choices of images you can build. The default Ubuntu image is most similar to the production servers but is larger in size and will take longer to build. Alternatively, you can use the Alpine image which is smaller, faster to setup, and should be compatible with the Ubuntu. To download the Ubuntu Docker images, build the Docker containers, seed the database with sample data, AND log the screen output from these tasks:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a note here that the Alpine image will need updating when we change the Ruby version in the future?

And I think it's safe to say, we don't have the resources to officially support both Ubuntu and Alpine, so we would not necessarily be supporting Alpine.

@dacook dacook added the blocked label Nov 13, 2024
@dacook
Copy link
Member

dacook commented Nov 13, 2024

Testing on Mac

I just tried building the default Ubuntu, and it failed after filling up my hard drive 😞 .

So after some cleanup and a system restart I tried the Alpine image. I now have more disk space (?! maybe because docker cleaned up the failed ubuntu image?). But it failed:

time docker/build alpine.Dockerfile
...
bundler: failed to load command: rake (/bundles/ruby/3.1.0/bin/rake)
/usr/local/src/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/bundler-2.4.3/lib/bundler/definition.rb:526:in `materialize': Could not find bigdecimal-3.1.8 in locally installed gems (Bundler::GemNotFound)
...
real	8m27.699s

Maybe that's because #12950 hasn't been merged yet. I've marked as blocked and set to 'In Progress' until that's merged, then we can rebase and try again.

@macanudo527
Copy link
Collaborator Author

Maybe that's because #12950 hasn't been merged yet. I've marked as blocked and set to 'In Progress' until that's merged, then we can rebase and try again.

This is the exact reason. We need to wait for that PR to pass and then I can rebase this with it and the error should go away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress ⚙
Development

Successfully merging this pull request may close these issues.

5 participants