Closed ajhodgson closed 1 year ago
Looks good ! How do I test this before it gets pushed to Dockerhub ? can I docker-pull your branch easily ?
I'm actually still newish to Docker. You can run make and it'll build all the images locally for you to run, though.
git clone gh:phusion/passenger-docker
cd passenger-docker/
git fork-checkout ajhodgson:ajhodgson/security-updates
make build_ruby27
....
Successfully tagged phusion/passenger-ruby27:2.4.0
Looks to be working fine for me. My app image is building fine on top. ~I could not test deployment at this time, but I will later.~ Successful deployment confirmed.
@CamJN can we get this merged and released?
@arni-wxnc there is unaddressed feedback, so not yet.
there is unaddressed feedback, so not yet.
Sorry, I must be missing something obvious, but where are the unaddressed issue(s)?
It would be good if someone could test the JRuby image; I don't have anything that uses it.
@ajhodgson my bad, apparently github code review comments are private and not visible to the PR author, seems dumb. Oh well, here's what I said:
The change in
image/jruby-9.3.9.0.sh
is wrong, per https://www.jruby.org/2022/10/24/jruby-9-3-9-0.html jruby < 9.4.0.0 is 2.6 compatible; and jruby >= 9.4.0.0 is 3.1 compatible per https://www.jruby.org/2022/11/23/jruby-9-4-0-0.html
@CamJN Would you prefer that image still installs CRuby 2.6 then? My understanding was that the CRuby in that image is just there for Passenger.
@ajhodgson or just bump jruby all the way to 9.4.0.0 so that the ruby3.1 wrapper script is accurate
@ajhodgson or just bump jruby all the way to 9.4.0.0 so that the ruby3.1 wrapper script is accurate
I could do that, but I didn't really want to get into possible Java updates and force a compatibility change.
@arni-wxnc there is unaddressed feedback, so not yet.
Apologies, as you've pointed out, I couldn't see the feedback :)
@CamJN I'm done done with this pending further review.
I was going to add a JRuby 9.4 image but it comes with some questions and would probably be better in a separate PR.
@CamJN any chance we can get this released?