-
Notifications
You must be signed in to change notification settings - Fork 400
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
Improving the Ruby Feature. #764
Conversation
keyserver hkp://pgp.mit.edu | ||
keyserver hkp://ipv4.pool.sks-keyservers.net | ||
keyserver hkp://keyserver.ubuntu.com | ||
keyserver hkp://keyserver.ubuntu.com:80" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the keyservers recommended in the RVM documentation in addition to the ones previously included.
@@ -10,12 +10,17 @@ | |||
"proposals": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing an os-provided Ruby is difficult.
The mcr.microsoft.com/devcontainers/base:bullseye image does not include Ruby. The Ruby devcontainer image (images/src/ruby/.devcontainer/Dockerfile
in the devcontainer/images repository) is based upon a Ruby image from the official Ruby Docker images and not an image in the Microsoft container registry, which does include an installed Ruby. This is why my (not yet committed) test and change to handle this os-provided option isn't working.
I'm even hesitant to include a "none" option. But I did aim for parity with the Node Feature, and it offers a none option which just installs NVM. So I carried that option over here. I did notice, though, that Node also has the issue where their devcontainer image is based on an official Node docker image and not an image in the MCR.
Copied from #757 (comment), although I did make a few additional (mostly minor) edits to this comment: @ThomasOwens Thank you so much for taking this on! To summarize my current thoughts here: As an incremental improvement, this is fine. But if we are going to bump the version number for backward incompatibility, I personally vote for delaying until we can make I personally consider the "minimum feature set" to be:
My recommendation on how to satisfy my "minimum feature set":
I'm not sure if it should be part of the "minimum feature set", but I think we should probably do one of the following, to aid with backward-incompatible upgrade pains:
None of the following are in my personal minimum feature set... but they would all be nice to have:
What do you think? (My apologies: I'll probably be too busy with other end-of-year stuff both at work and in my personal time, and I probably can't really offer any more than my criticism right now.) |
No changes (yet) should be breaking. This may change before the PR is merged.
No longer setting environment variables for RVM.
This is a work-in-progress draft pull request demonstrating some improvements to the Ruby feature to address #603 and #757.
In the current state, the Ruby Feature now has parity with the Node Feature. The issue where multiple Ruby version managers have been installed has been solved. Currently, RVM is used. Although I prefer rbenv (and the poll seems to perhaps favor rbenv as well), I noticed the Node Feature uses NVM and doesn't provide an option for nodenv or any other Node version managers. Since Node is frequently used with Ruby for web development (Rails, Sinatra), it seemed to make since to make the first step resolving the key problem and making it mimic the Node Feature.