Update Ruby version to 3.0.6 (latest of 3.0 series) #19603
Conversation
heliocola
requested review from
jaw6 and
dukegreene
and removed request for
a team
|
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
.env_sample
Outdated
| @@ -1,7 +1,7 @@ | |||
| # Development options | |||
|
|
|||
| # App core values | |||
| APP_DOMAIN="localhost:3000" | |||
| APP_DOMAIN="forem.test" | |||
There was a problem hiding this comment.
On GH Actions, some tests are still failing with invalid domain.
1st Try error in ./spec/system/feedback_message_spec.rb:13:
invalid domain: ".localhost:3000"
RSpec::Retry: 2nd try ./spec/system/feedback_message_spec.rb:13
2nd Try error in ./spec/system/feedback_message_spec.rb:13:
invalid domain: ".localhost:3000"
RSpec::Retry: 3rd try ./spec/system/feedback_message_spec.rb:13
There was a problem hiding this comment.
There was a problem hiding this comment.
@maestromac , @klardotsh : I just found in bin/setup that .env_sample is used as base for Dev/Local environment.
unless File.exist?(".env")
FileUtils.cp ".env_sample", ".env"
end
So, I need to find a different solution for test environment.
I can simply have a .env.test with a different APP_CONFIG or change environments/test.rb to not consider this.
I will do some more digging on this tomorrow.
If you have a preferred way to go, please let me know.
There was a problem hiding this comment.
Hi @heliocola — I'm not sure about this change (I'll defer to @maestromac and @klardotsh there) but the Ruby upgrade looks good without it, and this change seems unrelated. I'd suggest maybe moving this change to a separate PR, that way the Ruby upgrade can proceed on its own?
There was a problem hiding this comment.
This change is required because GCI does not allow 3000 to be part of cookie domain.
And if I am following this correctly, doesn't this mean the development cookie domain also needs to be updated? I can't see any error when I try this out locally though
There was a problem hiding this comment.
I think splitting the .env for testing is the best way to go.
I will push that change and sync up this PR.
@maestromac : for development environment, this variable isn't used for cookie the way it is for Testing and Production.
Check these references: https://github.com/forem/forem/blob/main/config/initializers/middlewares.rb#L4C89-L4C89
But it still doesn't explain how this fails in the testing environment.
I will split the env file, and do some digging if I remember how the test env is affected.
|
Forem Team: I reviewed all the CI failures and it looks like the issues with
The current CI failures are grouped as follow:
My main goal with this PR is to be able to get to latest version of Ruby 3.0 series and from there update to Ruby 3.1.2 (and do some benchmarking with YJIT). And at last but not least: there is an issue with cookies domain, as noted by @gareth here: #19523 (comment) That can probably be a separate issue to make Forem to conform with RFC 6265. Despite the fact Rails is changing its behavior (ref: rails/rails#48036 and thank you @gareth ), Forem repo does specifically adds leading dot to the domain in a few places, so that should be treated separately.
So that deserves its own issue, pr, and review process. |
|
@maestromac flagging you on this one |
|
Upgrading the
This may be a weird "I use ZFS on my host system" quirk, or it may be an issue with Void Linux's Docker builds, or it may be an outright problem with how we build images. I'd have to do some testing (and probably tap @maestromac, who is on Fedora with, I presume, ext4 or btrfs root fs, for more data points to narrow down the cause of failure) I suspect getting this PR merged is thus a minimum of a week of active (at 75%+ of a full-time in-house Forem engineer's available working time) effort. |
|
@klardotsh : thank you for looking into it and for the detailed comment. Ruby 3.0.* EOL is in less than 1 year (expected for 2024-03-31), and going to 3.0.6 was just a strategy to go to latest 3.0 before upgrading it to 3.1. I believe the changes in this PR (cgi and APP_DOMAIN without :3000) will be needed for Ruby 3.1 as well. |
|
Attempting to upgrade directly to Ruby 3.1+ does not solve point three above where the images currently do not build (at least for me, so far). That's something we need to figure out internally before we can take any upgrades to Ruby at all. I do think taking the final patch release of the 3.0 series before moving to 3.1 is a good idea in general, so I want to keep this PR going and advocate internally (I've already started these discussions) for making the time to unblock it in the ways mentioned above :) |
|
Additionally: for what it's worth, the |
There was a problem hiding this comment.
@heliocola I really appreciate you opening this PR! I'm mostly a 👍 but as mentioned, I'd prefer to split the proposed .env change into a separate PR so we can merge this one without delay.
.env_sample
Outdated
| @@ -1,7 +1,7 @@ | |||
| # Development options | |||
|
|
|||
| # App core values | |||
| APP_DOMAIN="localhost:3000" | |||
| APP_DOMAIN="forem.test" | |||
There was a problem hiding this comment.
Hi @heliocola — I'm not sure about this change (I'll defer to @maestromac and @klardotsh there) but the Ruby upgrade looks good without it, and this change seems unrelated. I'd suggest maybe moving this change to a separate PR, that way the Ruby upgrade can proceed on its own?
| @@ -173,3 +173,5 @@ group :test do | |||
| gem "with_model", "~> 2.1.6" # Dynamically build a model within an RSpec context | |||
| gem "zonebie", "~> 0.6.1" # Runs your tests in a random timezone | |||
| end | |||
|
|
|||
| gem "cgi", "~> 0.3.6" # Support for the Common Gateway Interface protocol. | |||
There was a problem hiding this comment.
For other reviewers:
- 3.0.5 addresses a response-splitting vulnerability in the CGI gem and encourages pinning it to a version >
0.3.5 - Also, 3.0.3 addresses a buffer overrun in the CGI gem
We haven't upgraded our Ruby in over a year 😱
|
Hey @heliocola, just letting you know that the infra team here at Forem (that's @maestromac and I) are digging into this actively. I don't yet know when we'll have this unblocked, but hopefully within the coming days to weeks. Steps we're taking:
I'll circle back when I know more. Thanks also to @jaw6 for the review above: I agree that I'd prefer to see |
|
Addendum: reffing #19626 as well here (the issue form of the discussion I linked above). |
This builds a Debian-derived base image that targets Ruby 3.0.2 (our current version lock) to replace `quay.io/forem/ruby`, which is based on Fedora 35 (and as such, lacks ARM64 support). The building of this image is not yet automated, I intend to add that as a follow-up some time vaguely soon-ish. This has been built and pushed to a temporary tag, [ghcr.io/forem/ruby:klardotsh-test](https://github.com/orgs/forem/packages/container/ruby/103882793?tag=klardotsh-test). Once this merges, I'll build and push the result in main (pending any revisions that come up in PR review) with the following incantation, which will generate a multiarch manifest: ```sh docker buildx build --platform linux/amd64,linux/arm64 -f Containerfile.base . -t ghcr.io/forem/ruby:latest -t ghcr.io/forem/ruby:$(git rev-parse --short HEAD) --push ``` This refs, but does not complete, #19626, and is one of several blockers on the path to getting #19603 merged.
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`.
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`.
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`.
|
@maestromac , @klardotsh , @jaw6 : TL'DR: I will close this PR and open a fresh one & clean one. The Long storyHere is a summary of my latest changes: 1. Removed the sym link on .env.test and create its own file.I've tried to change 2. Fixed/Changed some rubocop offensesDuo to changing some files for the item above, I've had to fix some rubocop offenses. click_button("button", text: /\ASave changes\z/)instead of find("button", text: /\ASave changes\z/).clickBut this change cause CI because click_button only works when button is disabled and serveral test were failing with: But reverting it, I have to wrestle with rubocop to commit the changes. |
…e. (#19632) This builds a Debian-derived base image that targets Ruby 3.0.2 (our current version lock) to replace `quay.io/forem/ruby`, which is based on Fedora 35 (and as such, lacks ARM64 support). The building of this image is not yet automated, I intend to add that as a follow-up some time vaguely soon-ish. This has been built and pushed to a temporary tag, [ghcr.io/forem/ruby:klardotsh-test](https://github.com/orgs/forem/packages/container/ruby/103882793?tag=klardotsh-test). Once this merges, I'll build and push the result in main (pending any revisions that come up in PR review) with the following incantation, which will generate a multiarch manifest: ```sh docker buildx build --platform linux/amd64,linux/arm64 -f Containerfile.base . -t ghcr.io/forem/ruby:latest -t ghcr.io/forem/ruby:$(git rev-parse --short HEAD) --push ``` This refs, but does not complete, #19626, and is one of several blockers on the path to getting #19603 merged.
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`.
…e. (#19632) This builds a Debian-derived base image that targets Ruby 3.0.2 (our current version lock) to replace `quay.io/forem/ruby`, which is based on Fedora 35 (and as such, lacks ARM64 support). The building of this image is not yet automated, I intend to add that as a follow-up some time vaguely soon-ish. This has been built and pushed to a temporary tag, [ghcr.io/forem/ruby:klardotsh-test](https://github.com/orgs/forem/packages/container/ruby/103882793?tag=klardotsh-test). Once this merges, I'll build and push the result in main (pending any revisions that come up in PR review) with the following incantation, which will generate a multiarch manifest: ```sh docker buildx build --platform linux/amd64,linux/arm64 -f Containerfile.base . -t ghcr.io/forem/ruby:latest -t ghcr.io/forem/ruby:$(git rev-parse --short HEAD) --push ``` This refs, but does not complete, #19626, and is one of several blockers on the path to getting #19603 merged.
…#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`.
…#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`.
What type of PR is this? (check all applicable)
Description
In preparation to try out Ruby 3.1.2 with YJIT, this PR udpate to latest version of the Ruby 3.0 series.
The main goal of this PR is to update to the latest 3.0 series and create an issue to discuss an strategy and benchmark for Ruby 3.1.2 with and without YJIT.
Related Tickets & Documents
This PR replaces #19523
QA Instructions, Screenshots, Recordings
No extra QA would be necessary, but let CI run would be ideal.
UI accessibility checklist
None
For more info, check out the
Forem Accessibility Docs.
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?