Skip to content

AVRO-3736: [Ruby] Preinstall gems in ubertool docker #2191

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.

Sign up for GitHub

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

Jump to bottom
Merged
RyanSkraba merged 3 commits into from
May 25, 2023
Merged

AVRO-3736: [Ruby] Preinstall gems in ubertool docker #2191

RyanSkraba merged 3 commits into from
May 25, 2023

Conversation

@RyanSkraba
Copy link
Contributor

@RyanSkraba RyanSkraba commented Apr 19, 2023

What is the purpose of the change

This pull request preinstalls the gems that the Avro Ruby SDK depends on in the ubertool docker container.

Verifying this change

This change was manually verified by building a clean docker and building the Avro Ruby SDK.

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@RyanSkraba RyanSkraba requested a review from tjwp April 19, 2023 07:17
@RyanSkraba
Copy link
Contributor Author

RyanSkraba commented Apr 19, 2023

I'm not very well-versed in Ruby development, so this is pretty naive -- I tried to follow the rbenv / old style when we use to preinstall gems in the docker ubertool by copying the necessary files to /tmp/lib/avro/.

tjwp
tjwp reviewed Apr 21, 2023
View reviewed changes
Copy link
Contributor

@tjwp tjwp left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this

share/docker/Dockerfile Outdated
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 && \
Copy link
Contributor

@tjwp tjwp Apr 21, 2023

Choose a reason for hiding this comment

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

bundler 1.17.3 is pretty old now. I tested your changes without specifying the version and everything still works.

Suggested change
RUN gem install bundler -v 1.17.3 --no-document && \
RUN gem install bundler --no-document && \

lang/ruby/Gemfile Outdated
Comment on lines 19 to 26
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
Copy link
Contributor

@tjwp tjwp Apr 21, 2023

Choose a reason for hiding this comment

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

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?

build.sh Outdated
# 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" -
Copy link
Contributor

@tjwp tjwp Apr 21, 2023

Choose a reason for hiding this comment

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

Suggest adding share/VERSION.txt here too:

Suggested change
tar -cf- Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" -
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" -

build.sh Outdated

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 |
Copy link
Contributor

@tjwp tjwp Apr 21, 2023

Choose a reason for hiding this comment

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

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:

Suggested change
tar -cf- share/docker/Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest |
tar -cf- share/docker/Dockerfile $EXTRA_DOCKER_CONTEXT |

Copy link
Contributor Author

@RyanSkraba RyanSkraba May 25, 2023

Choose a reason for hiding this comment

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

SGTM -- I chose $DOCKER_EXTRA_CONTEXT to match the other environment variables.

@RyanSkraba RyanSkraba force-pushed the rskraba/AVRO-3736-ruby-fix branch from c6cc4eb to 5ebcf50 Compare May 25, 2023 18:28
@github-actions github-actions bot removed the Ruby label May 25, 2023
@RyanSkraba
Copy link
Contributor Author

RyanSkraba commented May 25, 2023

Thanks for the PR review -- I didn't see it go by! I made the requested changed, and currently testing a clean build from my local machine.

@RyanSkraba RyanSkraba requested a review from tjwp May 25, 2023 18:30
tjwp
tjwp approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@tjwp tjwp left a comment

Choose a reason for hiding this comment

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

Looks good!

@RyanSkraba
Copy link
Contributor Author

RyanSkraba commented May 25, 2023

It built locally! Thanks for the review.

@RyanSkraba RyanSkraba merged commit 3dbf0a0 into apache:master May 25, 2023
@RyanSkraba RyanSkraba deleted the rskraba/AVRO-3736-ruby-fix branch May 25, 2023 19:38
RyanSkraba added a commit to RyanSkraba/avro that referenced this pull request Jun 13, 2023
@RyanSkraba @tjwp
AVRO-3736: [Ruby] Preinstall gems in ubertool docker (apache#2191)
eb8fe6a 
* AVRO-3736: [Ruby] Preinstall gems in ubertool docker

* Replicate expected directory structure

Co-authored-by: Tim Perkins <[email protected]>

* AVRO-3736: Update from PR comments

---------

Co-authored-by: Tim Perkins <[email protected]>
@RyanSkraba RyanSkraba mentioned this pull request Jun 13, 2023
Closed
RyanSkraba added a commit that referenced this pull request Jun 14, 2023
@RyanSkraba @tjwp
AVRO-3736: [Ruby] Preinstall gems in ubertool docker (#2191)
e3c5658 
* AVRO-3736: [Ruby] Preinstall gems in ubertool docker

* Replicate expected directory structure

Co-authored-by: Tim Perkins <[email protected]>

* AVRO-3736: Update from PR comments

---------

Co-authored-by: Tim Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tjwp tjwp tjwp approved these changes

Assignees

No one assigned

Labels

build

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RyanSkraba @tjwp