[GitHub] [avro] tjwp commented on a diff in pull request #2191: AVRO-3736: [Ruby] Preinstall gems in ubertool docker

["tjwp (via GitHub)" <gi...@apache.org>]

tjwp commented on code in PR #2191:
URL: https://github.com/apache/avro/pull/2191#discussion_r1173925828


##########
lang/ruby/Gemfile:
##########
@@ -16,8 +16,14 @@
 
 source 'https://rubygems.org'
 
-VERSION = File.open('../../share/VERSION.txt').read.sub('-SNAPSHOT', '.pre1').chomp
-File.write("lib/avro/VERSION.txt", VERSION)
+# Add the VERSION.txt if it exists, or use a fake one for preinstalling gems
+version_path = '../../share/VERSION.txt'
+if File.exist?(version_path)
+  VERSION = File.open(version_path).read.sub('-SNAPSHOT', '.pre1').chomp
+  File.write("lib/avro/VERSION.txt", VERSION)
+else
+  File.write("/tmp/lib/avro/VERSION.txt", "0.0.1-fake")
+end

Review Comment:
   Since the `VERSION.txt` file is kept in git maybe it is better to keep the original code here and ensure that `share/VERSION.txt` is copied like the Gemfile, gemspec, and Manifest?



##########
share/docker/Dockerfile:
##########
@@ -179,6 +179,11 @@ RUN python3 -m pip install --upgrade pip setuptools wheel \
 # Install Ruby
 RUN apt-get -qqy install ruby-full \
  && apt-get -qqy clean
+COPY lang/ruby/* /tmp/
+RUN gem install bundler -v 1.17.3 --no-document && \

Review Comment:
   bundler 1.17.3 is pretty old now. I tested your changes without specifying the version and everything still works.  
   
   ```suggestion
   RUN gem install bundler --no-document && \
   ```



##########
build.sh:
##########
@@ -300,8 +300,9 @@ do
         echo "RUN getent group $GROUP_ID || groupadd -g $GROUP_ID $USER_NAME"
         echo "RUN getent passwd $USER_ID || useradd -g $GROUP_ID -u $USER_ID -k /root -m $USER_NAME"
       } > Dockerfile
+      # Include the ruby gemspec for preinstallation.
       # shellcheck disable=SC2086
-      tar -cf- lang/ruby/Gemfile Dockerfile | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" -
+      tar -cf- Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" -

Review Comment:
   Suggest adding `share/VERSION.txt` here too:
   
   ```suggestion
         tar -cf- Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest share/VERSION.txt | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" -
   ```



##########
share/docker/Dockerfile:
##########
@@ -179,6 +179,11 @@ RUN python3 -m pip install --upgrade pip setuptools wheel \
 # Install Ruby
 RUN apt-get -qqy install ruby-full \
  && apt-get -qqy clean
+COPY lang/ruby/* /tmp/
+RUN gem install bundler -v 1.17.3 --no-document && \
+    apt-get install -qqy libyaml-dev && \
+    mkdir -p /tmp/lib/avro/ && \
+    bundle install --gemfile=/tmp/Gemfile

Review Comment:
   If the `share/VERSION.txt` is copied then here is a suggestion to replicate the expected directory structure and keep the `Gemfile` simpler (removal of the bundler version also included):
   
   ```suggestion
   RUN mkdir -p /tmp/lang/ruby/lib/avro && mkdir -p /tmp/share
   COPY lang/ruby/* /tmp/lang/ruby/
   COPY share/VERSION.txt /tmp/share/
   RUN gem install bundler --no-document && \
       apt-get install -qqy libyaml-dev && \
       cd /tmp/lang/ruby && bundle install
   ```



##########
build.sh:
##########
@@ -336,7 +337,7 @@ do
       ;;
 
     docker-test)
-      tar -cf- share/docker/Dockerfile lang/ruby/Gemfile |
+      tar -cf- share/docker/Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest |

Review Comment:
   Suggest adding `share/VERSION.txt` here too. To keep the list of extra files in sync between `docker` and `docker-test` targets it may be worth extracting a variable like:
   
   ```
   EXTRA_DOCKER_CONTEXT="lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest share/VERSION.txt"
   ```
   
   And then reference that here and above in the `docker` target:
   
   ```suggestion
         tar -cf- share/docker/Dockerfile $EXTRA_DOCKER_CONTEXT |
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org