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

bump base gem versions #122

Closed
wants to merge 6 commits into from
Closed

Conversation

JoeNunnelley
Copy link
Contributor

so I'm bumping bundler from 1.17 to 1.17.3 to get a fix for a bug that is affecting storyteller. rubygems/bundler#6761

also bumping the rubygems version to the latest 2.x version so they are all uniform.

Copy link

@rantler rantler left a comment

Choose a reason for hiding this comment

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

Just a couple changes needed re: the -v option to the gem install bundler commands.

py3_ruby/Dockerfile Outdated Show resolved Hide resolved
runit-ruby/2.3/Dockerfile Outdated Show resolved Hide resolved
@@ -1,40 +0,0 @@
FROM socrata/ruby:2.3.6

Choose a reason for hiding this comment

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

Hey, I see these files accidentally ended up multiple PRs. Are they now deleted locally so that doesn't keep happening?

@rantler
Copy link

rantler commented Apr 18, 2019

@JoeNunnelley Should we close this PR or is it still mergeable?

@JoeNunnelley
Copy link
Contributor Author

@rantler given updated knowledge I think it needs to be updated. Have to have both 1.x and 2.x bundler installed for compatibility. I'll update ASAP and get it merged. its a little bit involved to update these base images because it forces the rebuild of downwind images that we use to build things so it has to be coordinated.

@JoeNunnelley
Copy link
Contributor Author

@rantler can you look at this again. I've changed each of these to now have an update rubygems and to (where appropriate) have both a 1.x and 2.x bundler installed (for backwards compat).

Copy link

@rantler rantler left a comment

Choose a reason for hiding this comment

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

If you don't disagree, I think it's a good idea to be explicit about the 2.x version of bundler that we're also installing alongside the 1.17.x version.

@@ -97,6 +97,7 @@ RUN set -ex \
\
&& gem update --system "$RUBYGEMS_VERSION" \
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& gem install bundler
Copy link

Choose a reason for hiding this comment

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

Nit: This should probably be indented the same as the neighboring lines.

# skip installing gem documentation
RUN echo 'gem: --no-rdoc --no-ri --no-document' >> "/etc/gemrc" && \
gem install bundler -v 1.17 --no-document
gem update --system && \
Copy link

Choose a reason for hiding this comment

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

Nit: We're use two different styles in these different Dockerfiles:

# This one
gem update --system && \
gem install bundler && \

# And this one
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& gem install bundler

It's not a huge deal, but it would be nice to be consistent.

@@ -97,6 +97,7 @@ RUN set -ex \
\
&& gem update --system "$RUBYGEMS_VERSION" \
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& gem install bundler \
Copy link

Choose a reason for hiding this comment

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

Nit: Indentation

@@ -97,6 +97,7 @@ RUN set -ex \
\
&& gem update --system "$RUBYGEMS_VERSION" \
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& gem install bundler \
Copy link

Choose a reason for hiding this comment

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

Nit: Indentation. Sorry 😬 This is true in several files in this PR.

gem install bundler -v 1.17
gem update --system && \
gem install bundler --no-document -v 1.17.3 && \
gem install bundler --no-document
Copy link

Choose a reason for hiding this comment

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

We should probably be explicit about this version as well. This is the version 2.x of bundler, right?

@@ -97,6 +97,7 @@ RUN set -ex \
\
&& gem update --system "$RUBYGEMS_VERSION" \
&& gem install bundler --version "$BUNDLER_VERSION" --force \
&& gem install bundler \
Copy link

Choose a reason for hiding this comment

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

I recommend explicitly specifying the version of the newer bundler in these installs.

@JoeNunnelley
Copy link
Contributor Author

this is quite out of date given changes marc has been making. closing and will open a new PR if necessary

@JoeNunnelley JoeNunnelley deleted the jcn/bump_base_gem_versions branch August 8, 2019 21:19
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

3 participants