Generate Dockerfiles using distgen #259

mcyprian · 2018-04-04T09:03:18Z

Using distgen to generate Dockerfiles will decrease an amount of maintenance work on this project.
This is only the first step, I would like to render more files (directories) from the templates (see #250), more PRs are coming.

To generate Dockerfiles it is necessary to use the latest distgen from GitHub (not released yet), containing jinja2 behavior adjustment.

The generate-all makefile rule provided by common-scripts has to be accustomed to work for python container correctly. For now custom rule similar to this:

DG ?= /usr/bin/dg
DG_EXEC = ${DG} --max-passes 25 --template src/Dockerfile.template --multispec specs/multispec.yml

generate-dockerfiles:
    for version in ${VERSIONS}; do \
        ${DG_EXEC} --distro centos-7-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile; \
        ${DG_EXEC} --distro rhel-7-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile.rhel7; \
        ${DG_EXEC} --distro fedora-26-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile.fedora; \
    done

can be added to s2i-python-container's Makefile to check/review how this works. Fixes #244.

centos-ci · 2018-04-04T09:03:20Z

Can one of the admins verify this patch?

centos-ci · 2018-04-04T09:03:20Z

Can one of the admins verify this patch?

mcyprian · 2018-04-09T14:43:46Z

This was rebased after update of Fedora Dockerfile #262.
The makefile rule to generate Dockerfiles will now look like this:

DG ?= /usr/bin/dg
DG_EXEC = ${DG} --max-passes 25 --template src/Dockerfile.template --multispec specs/multispec.yml

generate-dockerfiles:
    for version in ${VERSIONS}; do \
        ${DG_EXEC} --distro centos-7-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile; \
        ${DG_EXEC} --distro rhel-7-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile.rhel7; \
        ${DG_EXEC} --distro fedora-27-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile.fedora; \
    done

torsava · 2018-04-10T10:52:16Z

[test]

torsava · 2018-04-10T10:56:33Z

2.7/Dockerfile

    rpm -e --nodeps centos-logos && \
-    yum clean all -y
+    yum -y clean all --enablerepo='*'


Why --enablerepo='*'?

This is how cleancache command is defined in distgen tool https://github.com/devexp-db/distgen/blob/master/distgen/commands.py#L72. I simply call it's method in the template cabe677#diff-bd0e57c1ca2f20c05a1456d4512e2eafR55.

I think difference is mainly in rhel7 Dockerfile.

For first yum call default repositories are enabled - so metadata are downloaded. Then prepare-yum-repositories disable these repositories... And yum clean cleans only enabled repositories, that's why --enablerepo='*' is needed.

torsava · 2018-04-10T11:22:23Z

src/Dockerfile.template

@@ -0,0 +1,73 @@
+{% import "src/common.tpl" as common with context %}
+{% import "src/" + config.os.id + "/macros.tpl" as macros with context %}
+{% if spec.version == "2.7" and config.os.id == 'centos' %}


Please check the if the spec.extra_pkgs variable exists (see spec.img_version below) instead of hardcoding config.os.id == 'centos'.

In addition, consider moving this definition of the extra_pkgs variable near where it is used in the template, so both the definition and usage is next to each other for better readability.

I like to have initialization of variable at the top of the file for some reason. The rest of the template have clean structure of the Dockerfile that is rendered from it thanks to this approach.

Ok, makes sense.

torsava · 2018-04-10T11:25:41Z

specs/multispec.yml

+      base_pkgs: ['nss_wrapper', 'httpd24', 'httpd24-httpd-devel', 'httpd24-mod_ssl',
+                  'httpd24-mod_auth_kerb', 'httpd24-mod_ldap', 'httpd24-mod_session',
+                  'atlas-devel', 'gcc-gfortran', 'libffi-devel', 'libtool-ltdl enchant']
+      extra_pkgs: ['libjpeg-turbo', 'libjpeg-turbo-devel']


Since extra_pkgs is only used for python27, would it be possible to create a dictionary here? For example:

extra_pkgs: - 2.7: ['libjpeg-turbo', 'libjpeg-turbo-devel']

That way we could hopefully access the variable as spec.extra_pkgs[spec.version].

If this turns out not to work, please rename the variable to be specific about the python version, for example python27_extra_pkgs, so the usage is clearer.

centos-ci · 2018-04-11T13:04:41Z

Can one of the admins verify this patch?

mcyprian · 2018-04-11T14:07:08Z

All the files in version subdirectories are now generated from templates stored under src/. Also the issue reported in #250 is resolved by this change.

torsava · 2018-04-12T09:38:29Z

[test]

torsava · 2018-04-12T09:50:56Z

specs/multispec.yml

+    "3.6":
+      version: "3.6"
+      short_ver: "36"
+      img_version: "1"


Could you move the "img_version" to the distro instead, ideally into a dictionary as suggested above, or as "python36_img_version" for better readability?

Img_version is not distro specific. it is used on more places not only the image tag mcyprian@cabe677#diff-c523d7243e565a463b4a2d07a9e2c40fR4. It was introduced in version 3.6 so it is logical to have it defined only for this version. After python 3.7 is added something like python36_img_version won't work.

Ah I see! I was confused because I saw it used like this below:

{% if config.os.id == 'rhel' and spec.img_version %}

Why is it limited to RHEL here?

Dockerfile for centos simply doesn't contain tag in FROM declaration https://github.com/mcyprian/s2i-python-container/blob/ultimate-distgen/3.6/Dockerfile#L3

mcyprian · 2018-04-12T10:31:29Z

[test]

omron93 · 2018-04-12T12:52:17Z

@mcyprian You have to set you membership in sclorg/ github org as public, to be able to trigger CentOS and Fedora CI.

[test]

torsava

Thanks for the PR, it's a lot of work!

torsava · 2018-04-16T09:18:16Z

specs/multispec.yml

+    "3.6":
+      version: "3.6"
+      short_ver: "36"
+      img_version: "1"


Ah I see! I was confused because I saw it used like this below:

{% if config.os.id == 'rhel' and spec.img_version %}

Why is it limited to RHEL here?

torsava · 2018-04-16T09:19:51Z

src/Dockerfile.template

@@ -0,0 +1,73 @@
+{% import "src/common.tpl" as common with context %}
+{% import "src/" + config.os.id + "/macros.tpl" as macros with context %}
+{% if spec.version == "2.7" and config.os.id == 'centos' %}


Ok, makes sense.

torsava · 2018-04-16T09:32:07Z

specs/multispec.yml

+
+specs:
+  distroinfo:
+    centos:


Is there some reason centos doesn't contain a version specifier (7) but rhel7 does? If there's no good reason, I think it'd be better to unify this one way or the other.

I copied it from multispec.yml postgresql-container useshttps://github.com/sclorg/postgresql-container/blob/master/specs/multispec.yml#L16, but you are right, it doesn't really make sense. I will try to use just rhel here.

torsava · 2018-04-16T09:32:25Z

src/fedora/macros.tpl

@@ -0,0 +1,24 @@
+{% macro env_metadata() %}


I see there's no src/rhel7/macros.tpl file. Is it not needed at all? I'd be inclined to at least create it as an empty file so it's more obvious to newcomers.

src/rhel is a symlink to src/centos https://github.com/sclorg/s2i-python-container/pull/259/files#diff-b43bada7e4bd74e88d661eaff149b323R1, the macros.tpl file is used for both centos and rhel. This import mcyprian@cabe677#diff-bd0e57c1ca2f20c05a1456d4512e2eafR2 wouldn't work otherwise.

Ah, I completely overlooked that, thanks!

torsava · 2018-04-16T09:34:40Z

src/centos/macros.tpl

+{% endmacro %}
+
+{% macro _prefix(spec) %}
+{% if spec.version.startswith('3.') %}{{ spec.component_prefix }}{% endif %}


Consider renaming to python3_component_prefix so this is clearer from the multispec.

torsava · 2018-04-16T09:41:16Z

src/centos/macros.tpl

+{% macro env_metadata() %}{% endmacro %}
+
+{% macro _version_selector(spec) %}
+{% if spec.img_version %}{{ spec.img_version }}{% else %}{{ spec.version }}{% endif %}


This might be a stupid question, so apologies in advance. Here it seems you're using img_version to refer to the version of the s2i-python-container, but here it's refering to the version of the base image. I think these should be two separate variables, I think it's just a coincidence that both these are currently 1 for python36 in RHEL. What do you think?

Yes you are right, I haven't realized this difference

torsava · 2018-04-16T10:02:34Z

src/common.tpl

+{% macro list_pkgs(pkgs) %}
+{% for n in range(0, pkgs|length, 5) %}
+{% if not loop.first %}        {% endif %}{{ ' '.join(pkgs[n:n+5]) }}{% if loop.last %}" &&{% endif %} \
+{% endfor %}


That's beautifully written!

torsava · 2018-04-16T10:14:20Z

src/Dockerfile.template

-{% else %}
-{%   set extra_pkgs = [] %}
-{% endif %}
+{% set ns = namespace(extra_pkgs=[]) %}


Wouldn't extra_pkgs = [] suffice? Why a namespace?

The extra_pkgs variable assigned inside for loop scope cannot be referenced outside the scope without the namespace

torsava · 2018-04-16T10:14:40Z

specs/multispec.yml

-      extra_pkgs: ['libjpeg-turbo', 'libjpeg-turbo-devel']
+      extra_pkgs:
+        - version: "2.7"
+          value: ['libjpeg-turbo', 'libjpeg-turbo-devel']


Beautiful, thank you!

torsava · 2018-04-16T12:37:38Z

src/test/pipenv-test-app/Pipfile.lock

@@ -0,0 +1,385 @@
+{% if spec.version == "3.6" %}


Wouldn't this be better to separate these into different files? That way if you want to regenerate it, you just overwrite the file instead of having to copy paste contents into this big jumble.

It probably would be better, unfortunately design of manifest.sh https://github.com/mcyprian/s2i-python-container/blob/ultimate-distgen/manifest.sh#L29 file doesn't allow to have different source files for different versions. The whole idea of manifest and generate rule is to generate everything from one collection of source files/templates.

As we talked, the different files could be included from this one.

torsava · 2018-04-17T08:56:56Z

src/Dockerfile.template

@@ -6,8 +6,8 @@
 {%     set ns.extra_pkgs = item.value %}
 {%   endif %}
 {% endfor %}
-{% if config.os.id == 'rhel' and spec.img_version %}
-{%   set img_tag = spec.img_version %}
+{% if config.os.id == 'rhel' and spec.base_img_version %}


If the base_img_version is only used for rhel, how about rhel_base_img_version to make it clearer? Sorry for nitpicking, I just hate to debug these little things so I like to avoid them if possible.

torsava · 2018-04-17T09:02:04Z

src/fedora/macros.tpl

@@ -0,0 +1,24 @@
+{% macro env_metadata() %}


Ah, I completely overlooked that, thanks!

torsava

Overall, looks great!

torsava · 2018-04-17T10:14:33Z

src/test/pipenv-test-app/2.7/pipfile.tpl

@@ -0,0 +1,96 @@
+{% macro content() %}
+{


Thanks! That's much better than having it in one file. However, would it be possible to solve without having to modify the individual Pipfile.lock files by adding this macro definition on top? In an ideal situation, one would just copy the Pipfile.lock file into the proper location, overwrite what's there and that'd be it. No modifying of the contents of the file.

Perhaps the include directive can be used in the src/test/pipenv-test-app/Pipfile.lock file?

{% include 'src/test/pipenv-test-app/" + spec.version + "/Pipfile.lock' %}

This also works

mcyprian · 2018-04-18T08:36:55Z

Rebased after merge of #267.

mcyprian · 2018-04-18T10:32:44Z

[test-openshift]

torsava · 2018-04-18T11:06:52Z

I went through the new commits and the previous modifications and everything looks in order.

One more thing: We should clarify the minimum jinja2 version requirement in the README alongside the distgen I think.

And after the container-common-scripts PR is merged, let's merge this.

mcyprian · 2018-04-18T12:15:23Z

Thanks for the review @torsava, I added note about jinja2 to README and squashed fixup commit.

mcyprian force-pushed the distgen branch 2 times, most recently from 119315e to cabe677 Compare April 9, 2018 14:40

mcyprian requested a review from torsava April 9, 2018 14:44

torsava reviewed Apr 10, 2018

View reviewed changes

torsava reviewed Apr 12, 2018

View reviewed changes

torsava requested changes Apr 16, 2018

View reviewed changes

This was referenced Apr 16, 2018

New release v1? devexp-db/distgen#77

Closed

Use distgen to get distro and version combinations sclorg/container-common-scripts#75

Merged

mcyprian force-pushed the distgen branch from 7afa720 to 4c52d92 Compare April 17, 2018 05:03

torsava reviewed Apr 17, 2018

View reviewed changes

mcyprian force-pushed the distgen branch 3 times, most recently from 7d3ffa2 to db7362c Compare April 18, 2018 08:35

torsava force-pushed the distgen branch from dbf2e10 to e17a5a6 Compare April 18, 2018 10:57

torsava approved these changes Apr 18, 2018

View reviewed changes

mcyprian added 2 commits April 18, 2018 13:30

Generate Dockerfiles using distgen

34bb9b3

Generate whole directories using distgen

5587a5a

mcyprian force-pushed the distgen branch from e17a5a6 to 5587a5a Compare April 18, 2018 12:09

mcyprian merged commit d53a0c2 into sclorg:master Apr 18, 2018

mcyprian deleted the distgen branch April 18, 2018 15:16

This was referenced Apr 24, 2018

Bring s2i scripts in line between versions #250

Closed

Process setup.py script by default #232

Merged

Change the working directory in run script to APP_HOME if set #228

Merged

Generate Dockerfiles using distgen #259

Generate Dockerfiles using distgen #259

Conversation

mcyprian commented Apr 4, 2018

centos-ci commented Apr 4, 2018

centos-ci commented Apr 4, 2018

mcyprian commented Apr 9, 2018

torsava commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

centos-ci commented Apr 11, 2018

mcyprian commented Apr 11, 2018 • edited Loading

torsava commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcyprian commented Apr 12, 2018

omron93 commented Apr 12, 2018

torsava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcyprian Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torsava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcyprian commented Apr 18, 2018

mcyprian commented Apr 18, 2018

torsava commented Apr 18, 2018

mcyprian commented Apr 18, 2018

mcyprian commented Apr 11, 2018 •

edited

Loading

mcyprian Apr 16, 2018 •

edited

Loading