devxp: Base the application container images off new Ruby base image. #19633
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm still building a "final pass" of this multiarch image locally, I'll add a link to the GHCR result later this evening / in the morning (and regardless, it'll be there before I reasonably suspect anyone will have reviewed this). EDIT: Looks like I have other Dockerfiles to go bump anyway, so marking this as a draft after all. |
|
|
klardotsh
force-pushed
the
klardotsh/rebase-app-container-on-new-ruby-container
branch
2 times, most recently
from
fe38bd5
to
0c0894f
Compare
Uffizzi Ephemeral Environment
|
This "rebases" our application images off of the new Ruby base image added in #19632, and fixes numerous problems and quirks with how the images were built along the way. Notably: - Issues where layers attempted to delete files in prior layers have been resolved (this caused build failures on some Docker filesystem drivers, notably overlay2). - Bundler is no longer allowed to deviate from or modify the lockfile (`BUNDLE_FROZEN` is now `true`). - `git(1)` is no longer required to live inside the container and `.git` is no longer required to be copied into the Docker build context, as these were only used to calculate `FOREM_BUILD_SHA`, which is now passed in as a Build Argument to the container build context. - The entire source tree is no longer `chmod` in one giant swing, which ran so long on my system (as just one example) that I gave up after 15-20 minutes and issued it a `SIGTERM`. Instead, `COPY --chown` is used more heavily and ensures the `APP_USER` will have access to the requisite files. This new container image appears to build successfully for `linux/arm64`, which refs (but does not complete) #19626. Currently, such builds aren't automated , and must be built on a developer workstation. For example: ```sh docker buildx build --platform linux/amd64,linux/arm64 -f Containerfile . -t ghcr.io/forem/forem:klardotsh-test --push --build-arg VCS_REF=$(git rev-parse --short HEAD) ``` In the meantime, the existing `linux/amd64`-only BuildKite scripts have been updated to allow this PR to merge as a separate unit, and CI refactors to enable the multiarch builds of `linux/arm64,linux/amd64` can come later when more time is available. This is one of several blockers on the path to getting #19603 merged. The next step in that chronology will be rebasing that work on top of this work, which *should* be, on the containerization side, as straightforward as bumping `Containerfile.base` to reference the new upstream image, rebuilding the base container, and then bumping the reference in `Containerfile`.
klardotsh
force-pushed
the
klardotsh/rebase-app-container-on-new-ruby-container
branch
from
0c0894f
to
39cc4c1
Compare
2 tasks
benhalpern
approved these changes
Jun 28, 2023
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #19633 +/- ##
==========================================
- Coverage 88.77% 88.71% -0.07%
==========================================
Files 1215 1215
Lines 27708 27708
Branches 2159 2159
==========================================
- Hits 24598 24581 -17
- Misses 2942 2959 +17
Partials 168 168
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
maestromac
approved these changes
Jun 29, 2023
| RUN bundle config --local build.sassc --disable-march-tune-native && \ | ||
| bundle config --delete without && \ | ||
| bundle install --deployment --jobs 4 --retry 5 && \ | ||
| BUNDLE_FROZEN=true bundle install --deployment --jobs 4 --retry 5 && \ |
There was a problem hiding this comment.
not applicable on this PR but for development, wouldn't we do the install without --deployment option?
There was a problem hiding this comment.
To do things like update Gemfile.lock, yeah, you'd want to do a one-off container like docker-compose run --rm web bundle update rails or whatever, skipping --deployment to force the lockfile to update. I think that's what you're asking here?
|
@klardotsh do you still want to lookover from @andygeorge or do you feel OK with the feedback you've received? |
|
This LGTM 👍! Let me know if you run into anything. |
klardotsh
deleted the
klardotsh/rebase-app-container-on-new-ruby-container
branch
rt4914
pushed a commit
that referenced
this pull request
Jun 29, 2023
…#19633) This "rebases" our application images off of the new Ruby base image added in #19632, and fixes numerous problems and quirks with how the images were built along the way. Notably: - Issues where layers attempted to delete files in prior layers have been resolved (this caused build failures on some Docker filesystem drivers, notably overlay2). - Bundler is no longer allowed to deviate from or modify the lockfile (`BUNDLE_FROZEN` is now `true`). - `git(1)` is no longer required to live inside the container and `.git` is no longer required to be copied into the Docker build context, as these were only used to calculate `FOREM_BUILD_SHA`, which is now passed in as a Build Argument to the container build context. - The entire source tree is no longer `chmod` in one giant swing, which ran so long on my system (as just one example) that I gave up after 15-20 minutes and issued it a `SIGTERM`. Instead, `COPY --chown` is used more heavily and ensures the `APP_USER` will have access to the requisite files. This new container image appears to build successfully for `linux/arm64`, which refs (but does not complete) #19626. Currently, such builds aren't automated , and must be built on a developer workstation. For example: ```sh docker buildx build --platform linux/amd64,linux/arm64 -f Containerfile . -t ghcr.io/forem/forem:klardotsh-test --push --build-arg VCS_REF=$(git rev-parse --short HEAD) ``` In the meantime, the existing `linux/amd64`-only BuildKite scripts have been updated to allow this PR to merge as a separate unit, and CI refactors to enable the multiarch builds of `linux/arm64,linux/amd64` can come later when more time is available. This is one of several blockers on the path to getting #19603 merged. The next step in that chronology will be rebasing that work on top of this work, which *should* be, on the containerization side, as straightforward as bumping `Containerfile.base` to reference the new upstream image, rebuilding the base container, and then bumping the reference in `Containerfile`.
jaw6
added a commit
that referenced
this pull request
Jun 30, 2023
…o-comment-notifications * origin/main: Standalone "admin" page for each comment (#19644) devxp: Base the application container images off new Ruby base image. (#19633) devxp: Base the application container images off new Ruby base image. (#19633) Add index to billboard placement area (#19648) Follow-ups for suggested organizations follows during onboarding (#19619) Fix bug: Unable to unfollow topics (#19637) Remove unnecessary fragment cache (#19653) [ruby] Update carrierwave 2.2.3 → 2.2.4 (patch) (#19614) devxp: Replace forem/ruby image with open-source Debian-based analogue. (#19632) Remove API dependency on fetching tweets and shift to frontend (#19641)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This strictly depends on #19632 being merged first.
This "rebases" our application images off of the new Ruby base image added in #19632, and fixes numerous problems and quirks with how the images were built along the way. Notably:
Issues where layers attempted to delete files in prior layers have been resolved (this caused build failures on some Docker filesystem drivers, notably overlay2).
Bundler is no longer allowed to deviate from or modify the lockfile (
BUNDLE_FROZENis nowtrue).git(1)is no longer required to live inside the containerand, as these were only used to calculate.gitis no longer required to be copied into the Docker build contextFOREM_BUILD_SHA, which is now passed in as a Build Argument to the container build context..gitis still copied into the build context for both image builds, it's just not necessary for the main app container build - I've left comments in the Uffizi Dockerfile to this end with a bit more color.The entire source tree is no longer
chmodin one giant swing, which ran so long on my system (as just one example) that I gave up after 15-20 minutes and issued it aSIGTERM. Instead,COPY --chownis used more heavily and ensures theAPP_USERwill have access to the requisite files.This new container image appears to build successfully for
linux/arm64, which refs (but does not complete) #19626. Currently, such builds aren't automated , and must be built on a developer workstation. For example:In the meantime, the existing
linux/amd64-only BuildKite scripts have been updated to allow this PR to merge as a separate unit, and CI refactors to enable the multiarch builds oflinux/arm64,linux/amd64can come later when more time is available.This is one of several blockers on the path to getting #19603 merged. The next step in that chronology will be rebasing that work on top of this work, which should be, on the containerization side, as straightforward as bumping
Containerfile.baseto reference the new upstream image, rebuilding the base container, and then bumping the reference inContainerfile.