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

Chown the directories added into container in test_from_dockerfile. #521

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

jackorp
Copy link
Contributor

@jackorp jackorp commented Feb 27, 2024

Since bundler v2.5.0, if the frozen option is not specified via CLI or config, bundler will 'touch' the Gemfile.lock to update timestamps. This will fail in test_from_dockerfile since the app-src is added into the container with ownership of root:root.

Ideally, we expect the directory is writeable, so adjust ownership via the Containerfile ADD --chown=: instruction.

Commit in bundler that introduced this behavior:
rubygems/rubygems@b08f2f1

Fixes for 3.3 test_from_dockerfile tests in #507 .

There are other alternatives, such as adding the --frozen, however supplying that is deprecated, so there would need to be an additional step that would use set and then we are depending on the behavior of bundler, which I personally don't like. I find it better to just own the directory. Other alternative is just chowning Gemfile.lock and leave the rest of the directory be.

Since bundler v2.5.0, if the `frozen` option is not specified via CLI or config,
bundler will 'touch' the Gemfile.lock to update timestamps. This will fail
in test_from_dockerfile since the app-src is added into the container with
ownership of root:root.

Ideally, we expect the directory is writeable, so adjust ownership via the
Containerfile ADD --chown=<user>:<group> instruction.

Commit in bundler that introduced this behavior:
rubygems/rubygems@b08f2f1
Copy link
Contributor

@zmiklank zmiklank left a comment

Choose a reason for hiding this comment

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

I am not sure why is the test run for every image if it only tests ubi8/ruby-27.
Am I missing something?

Otherwise LGTM. I would also consider bumping the FROM image, as ubi8/ruby-27 is not supported anymore

Ruby 2.7 is not supported, replace it with Ruby 3.1.
@jackorp
Copy link
Contributor Author

jackorp commented Feb 27, 2024

Am I missing something?

There be magic (and maybe dragons).

I checked the logs to not have to trace function in the test sources. Since we have -x we can see the following that it's doing in the following test_from_dockerfile excerpt from RHEL Ruby 3.3 Dockerfile test:

<...cut...>
+ sed -i -e 's|^FROM.*$|FROM ubi9/ruby-33:1|' Dockerfile
+ echo 'Using this Dockerfile:'
Using this Dockerfile:
+ cat Dockerfile
FROM ubi9/ruby-33:1

ADD app-src ./

RUN bundle install --path ./bundle

CMD exec bundle exec "rackup -P /tmp/rack.pid --host 0.0.0.0 --port 8080"
<...cut...>

So actually, the tests themselves change with sed the FROM without a care what was there before. Which makes sense, the tests create an image for themselves that we want to use to actually test the content of a PR.

This looks like the correct source: https://github.com/sclorg/container-common-scripts/blob/1e82b93382d056dd17b90b8e0e637e5bfb9cbb88/test-lib.sh#L1241

I would also consider bumping the FROM image, as ubi8/ruby-27 is not supported anymore

I don't think there is any danger to that, at least for our tests. Therefore I have done so and updated both Dockerfiles in the directory to use ubi8 Ruby 3.1

@zmiklank
Copy link
Contributor

There be magic (and maybe dragons).

Indeed! Thanks for the analysis.

@zmiklank zmiklank merged commit aabe7ee into sclorg:master Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants