WIP Initial support for parametrization of descriptor files #116

goldmann · 2017-05-02T20:36:31Z

Related to #104.

Consider following descriptor file:

version: 1.0
release: dev
name: jboss/test
from: {{FROM:rhel:7.3}}
packages:
    - make
scripts:
    - package: something
      exec: install

If you execute dogen with this new argument:

dogen -v --param FROM=centos:7 image.yaml target

dogen will generate this Dockerfile:

# Copyright 2017 Red Hat
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# ------------------------------------------------------------------------
#
# This is a Dockerfile for the jboss/test:1.0 image.

FROM centos:7

# Environment variables
ENV JBOSS_IMAGE_NAME="jboss/test" \
    JBOSS_IMAGE_VERSION="1.0" \
    JBOSS_IMAGE_RELEASE="dev" 
# Labels
LABEL name="$JBOSS_IMAGE_NAME" \
      version="$JBOSS_IMAGE_VERSION" \
      release="$JBOSS_IMAGE_RELEASE" \
      architecture="x86_64" \
      com.redhat.component="jboss-test-docker"

USER root


# Install required RPMs and ensure that the packages were installed
RUN yum install -y  make \
    && yum clean all && \
    rpm -q  make

# Add scripts used to configure the image
COPY scripts /tmp/scripts

# Custom scripts
USER 
RUN [ "bash", "-x", "/tmp/scripts/something/install" ]


USER root
RUN rm -rf /tmp/scripts

USER 0

If it is run without the --param switch it uses default values:

# Copyright 2017 Red Hat
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# ------------------------------------------------------------------------
#
# This is a Dockerfile for the jboss/test:1.0 image.

FROM rhel:7.3

# Environment variables
ENV JBOSS_IMAGE_NAME="jboss/test" \
    JBOSS_IMAGE_VERSION="1.0" \
    JBOSS_IMAGE_RELEASE="dev" 
# Labels
LABEL name="$JBOSS_IMAGE_NAME" \
      version="$JBOSS_IMAGE_VERSION" \
      release="$JBOSS_IMAGE_RELEASE" \
      architecture="x86_64" \
      com.redhat.component="jboss-test-docker"

USER root


# Install required RPMs and ensure that the packages were installed
RUN yum install -y  make \
    && yum clean all && \
    rpm -q  make

# Add scripts used to configure the image
COPY scripts /tmp/scripts

# Custom scripts
USER 
RUN [ "bash", "-x", "/tmp/scripts/something/install" ]


USER root
RUN rm -rf /tmp/scripts

USER 0

rcernich · 2017-05-02T22:16:00Z

This looks pretty good. I think it would also be beneficial to be able to specify these through a properties file. Using a file that's committed with the image.yaml, would provide a place where all the "externals" could be defined, so folks wanting to override some or all know what's been parameterized (e.g. properties section of a pom). I think supporting both command line options and properties file is good, with the options taking precedent over what's in any file. For something like community vs product images, there could be a centos.properties and a rhel.properties. Organizing in this way means the image.yaml contains all the structural elements of the image (scripts, artifacts, etc.), while the properties files identify the specifics (which artifacts, which base image, etc.).

goldmann · 2017-05-02T22:21:36Z

I think I'm ok with adding support for properties file. Stay tuned.

rwngwn · 2017-05-03T06:38:36Z

This seems weird to me dogen is based on jinja templates - so why second layer of parametrization?
Correct me if I get it wrong, but to be able to introduce new parameter I need to edit the template (it needs to know there is variable)?
If I need small changes there are already include mechanisms in jinja2 and project can maintain there complexities in their templates, putting it to upstream dogen seems very weird.
If we go with this and we will introduce a new parameter to template we should release a new version of dogen? This will probably generate more work than it saves.

rwngwn · 2017-05-03T06:47:08Z

dogen/cli.py

+                k, v = param.split("=", 1)
+                params[k] = v
+
+        args.params = params


this is very hacky - you should not mess with args object and store your values here.

Right. I guess the problem is that argparse doesn't support a dict directly. I could leave validation and just pass what I've got via CLI, but I guess I wanted to have the validation as close as possible to where argparse does it. I'm open for suggestions.

rwngwn · 2017-05-03T06:47:52Z

dogen/generator.py

-            self.cfg = yaml.safe_load(stream)
+            content = stream.read()
+
+            for k, v in six.iteritems(self.params):


in python 2.7 we don need six for this. do we support python 2.6 and so?

We don't support 2.6.

rwngwn · 2017-05-03T06:50:19Z

tests/test_dockerfile.py

+
+        generator = Generator(self.log, self.args)
+        generator.configure()
+        generator.render_from_template()


this code is copy pasted multiple times maybe it should be extracted to common setUp method

Probably a good idea, yes.

goldmann · 2017-05-03T12:30:41Z

@l-d-j Let's leave it here for a bit. We will see if supporting something like this is a good idea. We need some real use cases so we can make the correct decision. Until then this won't be merged. I'm happy to a discussion about this when we have some more information.

rwngwn · 2017-05-03T12:53:58Z

@goldmann +1 on tracking it and discussing it here. I still think we should look at jinja inheritance and include statements and try to solve via jinja.

rcernich · 2017-05-03T15:11:44Z

Here are some "real" use cases that we've received from teams migrating to dogen:

Substitute an artifact in the image with one built by a developer, e.g. a new version of kube-ping.
Use the same image.yaml for both community and product (e.g. from centos/from rhel; artifacts .Final/.Final-redhat-x)

As stated above, the yaml describes the basic structure and content of the image and the parameters allow for substitution of minor details. From a maintenance perspective, e.g. updating artifact versions, this would entail a change to the properties file, without having to change the yaml itself.

With respect to managing a template for this kind of stuff, I'd start to wonder why I'm using this over just managing a Dockerfile directly. I know docker; now I need to learn dogen and jinja.

The templates raise another issue in that, since we are using dogen to generate the Dockerfile, it means that the version of dogen needs to be specific to the version of the source in order to maintain repeatability of builds.

goldmann · 2017-05-05T09:59:04Z

@rcernich Regarding first use case - you can take a look at this idea: #117 (comment). Additional requirements are:

Skip hash checking to make it easy to do development work
Use target name for the artifact to make it easier to use a "stable" name in the scripts that use this artifact.

Initial support for parametrization of descriptor files

6b74adf

goldmann force-pushed the params branch from 9e03e01 to 6b74adf Compare May 2, 2017 20:50

goldmann added 2 commits May 2, 2017 17:02

Fix tests after adding 'params' argument

a09be91

Add tests for new --param argument

d9f0178

goldmann force-pushed the params branch from cc1b59a to d9f0178 Compare May 2, 2017 21:56

rwngwn reviewed May 3, 2017

View reviewed changes

goldmann added the discussion label Jun 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Initial support for parametrization of descriptor files #116

WIP Initial support for parametrization of descriptor files #116

goldmann commented May 2, 2017 •

edited

Loading

rcernich commented May 2, 2017

goldmann commented May 2, 2017

rwngwn commented May 3, 2017

rwngwn May 3, 2017

goldmann May 3, 2017

rwngwn May 3, 2017

goldmann May 3, 2017

rwngwn May 3, 2017

goldmann May 3, 2017

goldmann commented May 3, 2017

rwngwn commented May 3, 2017

rcernich commented May 3, 2017

goldmann commented May 5, 2017

WIP Initial support for parametrization of descriptor files #116

Are you sure you want to change the base?

WIP Initial support for parametrization of descriptor files #116

Conversation

goldmann commented May 2, 2017 • edited Loading

rcernich commented May 2, 2017

goldmann commented May 2, 2017

rwngwn commented May 3, 2017

rwngwn May 3, 2017

Choose a reason for hiding this comment

goldmann May 3, 2017

Choose a reason for hiding this comment

rwngwn May 3, 2017

Choose a reason for hiding this comment

goldmann May 3, 2017

Choose a reason for hiding this comment

rwngwn May 3, 2017

Choose a reason for hiding this comment

goldmann May 3, 2017

Choose a reason for hiding this comment

goldmann commented May 3, 2017

rwngwn commented May 3, 2017

rcernich commented May 3, 2017

goldmann commented May 5, 2017

goldmann commented May 2, 2017 •

edited

Loading