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

Revert RUBY_MAJOR refactoring for 3.0 #447

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

tianon
Copy link
Member

@tianon tianon commented Apr 23, 2024

(since it's effectively EOL now except the possibility of a severe regression: https://www.ruby-lang.org/en/news/2024/04/23/ruby-3-0-7-released/)

See #435

@tianon
Copy link
Member Author

tianon commented Apr 23, 2024

Hmmm, as I look at this, 3.1 is also imminently EOL, so I think my opinion on this is that we should only revert the change for 3.0 and let 3.1 stay as-is (esp. so we can remove the conditional after 3.0 is removed and any downstream breakage from this should be pretty minimal).

(since it's effectively EOL now except the possibility of a severe regression: https://www.ruby-lang.org/en/news/2024/04/23/ruby-3-0-7-released/)
@tianon tianon changed the title Revert RUBY_MAJOR refactoring for security updates Revert RUBY_MAJOR refactoring for 3.0 Apr 23, 2024
@tianon
Copy link
Member Author

tianon commented Apr 23, 2024

Updated.

@tianon
Copy link
Member Author

tianon commented Apr 23, 2024

@yosifkit is AFK at the moment, but he's ACKed the approach/choices ❤️

@tianon tianon merged commit 91faf12 into docker-library:master Apr 23, 2024
25 checks passed
@tianon tianon deleted the revert-breaks-security branch April 23, 2024 17:22
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Apr 23, 2024
Changes:

- docker-library/ruby@91faf12: Merge pull request docker-library/ruby#447 from infosiftr/revert-breaks-security
- docker-library/ruby@1db5a7b: Revert RUBY_MAJOR refactoring for 3.0
@@ -50,13 +50,13 @@ RUN set -eux; \
} >> /usr/local/etc/gemrc

ENV LANG C.UTF-8
{{ if .version | . == "3.0.6" or . == "3.1.4" or . == "3.2.2" then ( -}}
Copy link

Choose a reason for hiding this comment

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

@tianon This made our RUBY_MAJOR env var we're relying on in our Dockerfile vanish. We're using this env var for setting the BUNDLE_BIN_PATH (e.g. vendor/bundle/ruby/3.1.0/...). So we were using ruby 3.1.4 and now with 3.1.5, the BUNDLE_BIN_PATH became vendor/bundle/ruby/.0. Sure, I can workaround this, but the change seems wrong and it might affect others!

Copy link

Choose a reason for hiding this comment

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

the implementation is a bit weird anyway, we thought RUBY_MAJOR (that is 3.0, 3.1, 3.2. 3.3 ...) has some kind of significance in any RUBY_VERSION (e.g. 3.1.4 or 3.1.5)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the breakage, but it is a variable we never intended to expose (and with a name that isn't really accurate for what it is).

Copy link

Choose a reason for hiding this comment

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

didn't know and who else might not know it. we're deploying a fix right now. thanks

@mfittko
Copy link

mfittko commented Apr 25, 2024

Sorry for being a bit harsh earlier on, I was just surprised how the pull of the RUBY_MAJOR env var did not satisfy a changelog and refering to the PR description it might indeed have been unintended. I was a bit confused about the process. I know that it might be just wrong to rely on env variable being set in a base image to be still available even with a minor version change. You proved me wrong, thanks. We should have had tests ourself, I get it!

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.

None yet

2 participants