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

Document usage with a Dockerfile #295

Merged
merged 1 commit into from
Dec 21, 2020
Merged

Document usage with a Dockerfile #295

merged 1 commit into from
Dec 21, 2020

Conversation

fila43
Copy link
Member

@fila43 fila43 commented Oct 20, 2020

This adds the following:

  • Dockerfile examples for a variant that uses s2i scripts and a variant without s2i scripts
  • Documentation enhancement that describes how the package can be used using a Dockerfile
  • a test for the example Dockerfiles

This is a similar pull-request as sclorg/httpd-container#99., sclorg/nodejs-container#247 and sclorg/perl-container#192

To see the formatted README.md, see https://github.com/fila43/s2i-ruby-container/blob/master/2.5/README.md and it is also visible on the catalog, which will be the most important view: https://catalog.redhat.com/software/containers/rhel8/ruby-25/5ba0ae3bbed8bd6ee81985ea

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

2 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@fila43
Copy link
Member Author

fila43 commented Oct 20, 2020

[test]

@pvalena
Copy link
Member

pvalena commented Oct 20, 2020

The tests are failing on pulling images. Probably an error with common scripts?

https://gist.github.com/rhscl-automation/411a5d6c9a3c16dd8e615fe89601e18f#file-s2i-ruby-container-222-sh-L10891

@pkubatrh @phracek could you take a look? I'm not aware of any change in this repo. (Maybe it's some other change in this repo that I've missed).

Copy link
Member

@pvalena pvalena left a comment

Choose a reason for hiding this comment

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

Firstly, thank you for your PR.

I've commented on files for 2.5, but that applies for all versions. Please check those issues.
In general it looks good!

2.5/README.md Outdated Show resolved Hide resolved
2.5/README.md Show resolved Hide resolved
2.5/README.md Outdated Show resolved Hide resolved
2.5/README.md Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
FROM ubi8/ruby-25
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this file be in the 2.5 directory of this repo, not to repeateat another directory structure?

Copy link
Member Author

@fila43 fila43 Oct 21, 2020

Choose a reason for hiding this comment

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

It's an example and it belongs into the example directory, versions are there only due to tests

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but with Ruby f.e. we call this a fixture and it belongs right next to the tests.... I don't see why to add versioned examples when we have already versioned folders. It seems like enforced structure, because you wan't examples separated? But then again, shouldn't those be there for the ppl to use as a reference with the container? (Do they somehow iterfere with the resulting container?)

Copy link
Member

Choose a reason for hiding this comment

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

I'm still against versioned examples, inside this repo, when we have already versioned folders for the respective Ruby versions. @hhorak @voxik WDYT?

What I think:

  1. The Ruby Dockerfile examples should not be versioned. Same way as the example application is not versioned.
  2. If it's aimed only on testing, it should be inside the Versioned (Ruby) folder, inside either test or fixtures folder (see above).

Note that if there's a worry to have an actual Dockerfile which is built for the specific version, we can use Docker ARGs, like this:

https://github.com/pvalena/tmt-all/blob/master/Dockerfile#L1

Build would look like:

$ podman build --build-arg FROM='27' .
# or
$ podman build --build-arg FROM='ubi8/ruby-27' .
# or
$ podman build --build-arg FROM='ruby-27' .

Depending on what remains in Dockerfile.

Copy link
Member Author

@fila43 fila43 Dec 7, 2020

Choose a reason for hiding this comment

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

@pvalena, yes due to removing versioned examples I prepared PR #300 which enables SCL variables by default, and then we don't need versioned tests and dockerfiles

Copy link
Member

Choose a reason for hiding this comment

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

Great!

2.6/test/run Outdated Show resolved Hide resolved
2.5/README.md Show resolved Hide resolved
2.5/README.md Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
2.5/test/run Outdated Show resolved Hide resolved
@fila43
Copy link
Member Author

fila43 commented Oct 30, 2020

[test]

@pvalena
Copy link
Member

pvalena commented Oct 31, 2020

@fila43 Please incorporate this change: #295 (comment)

@fila43
Copy link
Member Author

fila43 commented Nov 3, 2020

@pvalena I have applied your suggestions from #295 (comment)

@fila43
Copy link
Member Author

fila43 commented Nov 3, 2020

[test]

2.5/Dockerfile.rhel7 Outdated Show resolved Hide resolved
2.5/README.md Outdated Show resolved Hide resolved
2.6/README.md Outdated Show resolved Hide resolved
2.6/README.md Outdated Show resolved Hide resolved
2.5/README.md Outdated Show resolved Hide resolved
@fila43
Copy link
Member Author

fila43 commented Nov 11, 2020

[test]

@fila43 fila43 closed this Nov 12, 2020
@pvalena
Copy link
Member

pvalena commented Nov 12, 2020

@fila43 closed by mistake?

@fila43
Copy link
Member Author

fila43 commented Nov 12, 2020

@pvalena yes, I am going to reopen it.

@pvalena pvalena reopened this Nov 13, 2020
@pvalena
Copy link
Member

pvalena commented Nov 13, 2020

(For a minute I thought you've opened another PR with this content.)

Could do rebase + force push? That's IMO a good practise for any PR.

$ git rebase --interactive

Is a great helper with that.

@phracek
Copy link
Member

phracek commented Dec 2, 2020

I have merged #301. Please rebase it. The conu tests should be fixed by this PR.

@fila43
Copy link
Member Author

fila43 commented Dec 2, 2020

@phracek thanks

Copy link
Member

@pvalena pvalena left a comment

Choose a reason for hiding this comment

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

I've proposed previously discussed text reformulation (sorry the suggestion took so long), together with fixes for the links (they're broken).

I'm also against using versioned examples-dockerfile (or something like that) folder, together with proposal for the location change. There're several ways to solve first mentioned.

2.5/README.md Outdated
* installing the dependencies
* setting the default command in the resulting image

For all these three parts, users can either setup all manually and use commands `ruby`, `bundle` and `rackup` explicitly in the Dockerfile ([3.1.](#31-to-use-your-own-setup-create-a-dockerfile-with-this-content)), or users can use the Source-to-Image scripts inside the image ([3.2.](#32-to-use-the-source-to-image-scripts-and-build-an-image-using-a-dockerfile-create-a-dockerfile-with-this-content); see more about these scripts in the section "Source-to-Image framework and scripts" above), that already know how to set-up and run some common Ruby applications.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For all these three parts, users can either setup all manually and use commands `ruby`, `bundle` and `rackup` explicitly in the Dockerfile ([3.1.](#31-to-use-your-own-setup-create-a-dockerfile-with-this-content)), or users can use the Source-to-Image scripts inside the image ([3.2.](#32-to-use-the-source-to-image-scripts-and-build-an-image-using-a-dockerfile-create-a-dockerfile-with-this-content); see more about these scripts in the section "Source-to-Image framework and scripts" above), that already know how to set-up and run some common Ruby applications.
For all these three parts, users can either setup all manually and use commands `ruby`, `bundle` and `rackup` explicitly in the Dockerfile ([3.1.](#31-to-use-the-source-to-image-scripts-and-build-an-image-using-a-dockerfile-create-a-dockerfile-with-this-content)), or users can use the Source-to-Image scripts inside the image ([3.2.](#32-to-use-your-own-setup-create-a-dockerfile-with-this-content).

Fixed links.
Also moved the last sentence to appropriate section bellow.

2.5/README.md Show resolved Hide resolved
@@ -0,0 +1,10 @@
FROM ubi8/ruby-25
Copy link
Member

Choose a reason for hiding this comment

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

I'm still against versioned examples, inside this repo, when we have already versioned folders for the respective Ruby versions. @hhorak @voxik WDYT?

What I think:

  1. The Ruby Dockerfile examples should not be versioned. Same way as the example application is not versioned.
  2. If it's aimed only on testing, it should be inside the Versioned (Ruby) folder, inside either test or fixtures folder (see above).

Note that if there's a worry to have an actual Dockerfile which is built for the specific version, we can use Docker ARGs, like this:

https://github.com/pvalena/tmt-all/blob/master/Dockerfile#L1

Build would look like:

$ podman build --build-arg FROM='27' .
# or
$ podman build --build-arg FROM='ubi8/ruby-27' .
# or
$ podman build --build-arg FROM='ruby-27' .

Depending on what remains in Dockerfile.

@fila43 fila43 force-pushed the master branch 4 times, most recently from bee7d32 to 751a660 Compare December 14, 2020 13:52
@fila43
Copy link
Member Author

fila43 commented Dec 14, 2020

[test]

@fila43
Copy link
Member Author

fila43 commented Dec 14, 2020

[test]

@fila43
Copy link
Member Author

fila43 commented Dec 15, 2020

@pvalena I hope now it's ready

@fila43
Copy link
Member Author

fila43 commented Dec 15, 2020

[test]

1 similar comment
@fila43
Copy link
Member Author

fila43 commented Dec 16, 2020

[test]

Copy link
Member

@pvalena pvalena left a comment

Choose a reason for hiding this comment

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

My only comment is the one bellow, but that can be handled separately.

Thanks!

2.5/test/run Show resolved Hide resolved
Edit documentation - add section from dockerfile
Enable testing from dockerfile example app (rails-ex)
Update common scripts to master
@fila43
Copy link
Member Author

fila43 commented Dec 21, 2020

[test]

@pvalena pvalena merged commit 2d64ca7 into sclorg:master Dec 21, 2020
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.

6 participants