More distgen - directory generation #203

dhodovsk · 2017-11-02T09:02:10Z

In this PR we added ability to generate directories 9.4, 9.5 and 9.6 using distgen.

Directories are generated using ./src directory and with command make generate

pkubatrh · 2017-11-02T09:15:12Z

9.4/root/usr/share/container-scripts/postgresql/README.md

@@ -51,49 +51,49 @@ Environment variables and volumes
 The image recognizes the following environment variables that you can set during
 initialization by passing `-e VAR=VALUE` to the Docker run command.

-**`POSTGRESQL_USER`**  


These trailing spaces are actually left there on purpose.

See: #185 (comment)

Not only that, white-space changes just cause rush in pull-requests. Please consider fixing your editor to not remove trailing white-spaces automatically (submit new PR for white-space issues, when needed)

pkubatrh · 2017-11-02T09:16:20Z

gen-dirs.sh

+set -e
+cd templates
+FILES=$(find -follow)
+VERSIONS="9.4 9.5 9.6"


VERSIONS should get passed to the script from the Makefile, so that the list can be maintained in one place.

pkubatrh · 2017-11-02T09:24:54Z

Since we agreed to remove the generated files in future we should make sure that we do not lose all of the git history by moving latest into template in a separate commit in this PR.

pkubatrh · 2017-11-02T09:25:37Z

(not sure if we even need latest after moving it to template)

pkubatrh · 2017-11-02T10:49:32Z

moving latest into template in a separate commit in this PR

To have an idea what I mean by this please check how latest/ was introduced: #170

dhodovsk · 2017-11-03T10:03:07Z

We might also want to preserve Dockerfile history therefore there are now two more commits to make that happen.

pkubatrh · 2017-11-03T14:24:55Z

Makefile

@@ -2,6 +2,7 @@
 BASE_IMAGE_NAME = postgresql
 VERSIONS = 9.4 9.5 9.6
 OPENSHIFT_NAMESPACES = 9.2
+export VERSIONS


I am a bit scared exporting VERSIONS might break something in the existing scripts. It would be better if you just kept the versions variable local for the gen-dirs.sh script. So something like:

VERSIONS="$(VERSIONS)" ./gen-dirs.sh

pkubatrh · 2017-11-03T14:25:30Z

We might also want to preserve Dockerfile history therefore there are now two more commits to make that happen.

That is true, thanks for picking up on this!

praiskup · 2017-11-09T15:02:19Z

Makefile

-		${DG_EXEC} --distro rhel-7-x86_64.yaml --multispec-selector version=$${version} --output $${version}/Dockerfile.rhel7; \
-	done
+distgen:
+	VERSIONS="$(VERSIONS)" ./gen-dirs.sh


Please use make targets instead of this script, and please develop them in container-common-scripts project ...

Running this script takes ~12s on my box, because distgen is pretty slow TBH:

$ make distgen real 0m11.999s user 0m11.124s sys 0m0.827s

(at least at this point). make should help with building it in parallel.

I agree. Adding the rules to container-common-scrtips will make the distgen change much easier to adopt in other images if the maintainers choose to do so.

praiskup · 2017-11-09T15:10:09Z

gen-dirs.sh

+    /usr/bin/dg --max-passes 25 --template ${PWD}/../Dockerfile.template \
+        --multispec ../specs/multispec.yml --distro rhel-7-x86_64.yaml \
+        --multispec-selector version=$1 --output ../$1/Dockerfile.rhel7
+


I'm not sure I follow right, but I think the goal is to remove the generated products from this repo, right?

I would really prefer to not list centos/rhel dockerfiles here, and anywhere else. We'll have fedora dockerfiles soon, and maybe scls+fedora? Who knows.. So it would be nice if, when generating Dockerfile, the $OS variable was respected.

praiskup · 2017-11-09T15:14:38Z

gen-dirs.sh

+#!/bin/bash
+set -e
+cd templates
+FILES=$(find -follow)


Hm, I dislike this, tbh. Especially that we filter everything through the build script, while simple copy would often be enough.

Maybe we could separate the real templates into ./templates, and normal files under ./files?

Other approach could be using some "manifest file", I implemented something like this in https://github.com/devexp-db/cont-postgresql/blob/master/cl-manifest
That file declars what should be copied, what should be "instantiated" (and by what tool) and everything explicitly (not copying some accidentally occurred files). This should stay executed by postgresql "in parallel" if implemented.

praiskup · 2017-11-09T15:17:23Z

gen-dirs.sh

+        mkdir ../${version}/$1
+    else
+        echo "Generating: ${version}/$1"
+        /usr/bin/dg --max-passes 1 --multispec ../specs/multispec.yml \


Once this is moved to Makefile (common.mk probably), please ensure that instead of /usr/bin/dg you are using $(DG) variable; to allow anybody to replace the distgen version from local installation (virtualenv).

dhodovsk · 2017-12-14T09:03:41Z

Blocked by sclorg/container-common-scripts#38

praiskup · 2018-01-04T11:28:18Z

manifest.sh

+# Manifest for image directories creation
+# every dest and link_name path will be prefixed by $DEST_DIR/$version
+
+DEST_DIR='' # optional, defaults to $PWD


s/DEST_DIR/DESTDIR/ -> that's far more standard variable

praiskup · 2018-01-04T11:36:48Z

manifest.sh

+
+    src=templates/root/usr/share/container-scripts/postgresql/common.sh
+    dest=root/usr/share/container-scripts/postgresql/common.sh
+    mode=0755


This doesn't have to be executable.

praiskup · 2018-01-04T11:38:58Z

manifest.sh

+
+    link_target=../test
+    link_name=test
+"


This seems to create absolute symlinks on the host system during build. Such simlinks - if copied into image - will lead to non-existing paths.

praiskup · 2018-01-04T11:39:47Z

manifest.sh

+
+    src=templates/root/usr/share/container-scripts/postgresql/scl_enable
+    dest=root/usr/share/container-scripts/postgresql/scl_enable
+    mode=0755;


I don't think this needs to be executable.

praiskup · 2018-01-04T11:53:36Z

I really like the speed of the generated output
I love the fact that we can adjust permissions of particular files! (the current approach doesn't ensure the proper permissions, but the following point should solve it...)
I miss dealing with Dockerfile.rhel7 - it is not re-created if I drop it (probably we should drop everything generated from the source directory)
ccpp.yml is not geneerated
I would not use the templates/ directory, but rather source/ or src/ (not everything under that is a jinja/distgen template)

For me, I'm leaning towards +1, but not without another positive votes.

phracek · 2018-01-08T13:26:16Z

The PR looks good to me. Good job @dhodovsk .
I am fine with merging. I would prefer to ACK it and create another PR for rest issues.

WDYT?

TomasTomecek · 2018-01-08T13:36:09Z

Wow, I'm really impressed how far distgen got. The manifest is some really good work (I would personally prefer json/yaml + python, but if the proposed implementation suits your needs, that's just fine).

I also agree with Pavel the time will tell. Hence I'm +1 on merging and addressing issues as they come.

dhodovsk · 2018-01-09T11:12:57Z

The requested changes should be now applied.

I'd like to discuss the possibility to drop directories that can be generated. I think we should do it to prevent the case someone commits changes in generated files (and not src). But it seems like a big step and we have to make sure it is safe.

dhodovsk · 2018-03-12T12:09:17Z

I am now rebasing and adding following changes:

add https://github.com/container-images/tools/blob/master/hooks/pre_build_centos script to new directory: ./hooks in root of this repository
append line prebuild-script: hooks/pre_build_centos to src/cccp.yaml

Is that enough? @TomasTomecek @pkubatrh

TomasTomecek · 2018-03-12T16:12:21Z

It should be. I don't know if the cccp.yaml file present in git repo overrides the one defined in cccp index git repo.

pkubatrh · 2018-03-13T07:31:02Z

Is there a way to test things out with the current configuration before merging the changes?

pkubatrh · 2018-03-13T08:15:51Z

Now that I look at it more closely. Will the prebuild script be even run if the yaml file in the container-index point to versioned directories?

If I understand this correctly cccp expects to find the cccp.yaml file over there and will not look for it in src/, or anywhere else in the matter. And then cccp wont know it should first run the hook to generate the files.

pkubatrh · 2018-03-13T08:18:22Z

hooks/pre_build_centos

+yum install distgen make -y
+
+mkdir -p root/
+make VARIANT=centos DISTRO=centos-7-x86_64 source


Probably needs to be make TARGET=centos7 generate? We also do not need the root/ directory creation.

pkubatrh · 2018-03-13T08:22:54Z

Actually according to https://github.com/CentOS/container-index/blob/master/README.md the cccp.yml file does not seem to even accept the prebuild-script configuration option. So I guess we will have to change the config in the index.

dhodovsk · 2018-03-13T08:28:15Z

The commit Add prebuild-script to cccp.yml is now reverted. Please, let me know if there is anything else I could do.

pkubatrh · 2018-03-13T08:37:04Z

The commit Add prebuild-script to cccp.yml is now reverted. Please, let me know if there is anything else I could do.

I think we need at least the hook part of the commit (the actual script I commented on), since the versioned directories will still need to be generated.

Its just that the configuration option to run it will need to be added elsewhere - similarly to https://github.com/container-images/tools

dhodovsk · 2018-03-13T08:52:08Z

I am sorry, but I have no insight in functionality and importance of changes related to cccp, so I am not able to decide the right workflow, could you please provide more info of what changes exactly are needed so the requirements are met? Or maybe provide a commit with those changes?

pkubatrh · 2018-03-13T09:02:59Z

I have added a commit with what I believe is needed for the cccp to work once changes are made in https://github.com/CentOS/container-index

Now the question is, do we merge this in first and make the changes in index later? The other option is to merge this PR in without the removal of versioned directories so that cccp works both with the old and with new configuration option (prebuild-script).

@hhorak @praiskup @omron93 WDYT?

omron93 · 2018-03-13T09:37:58Z

I would merge this PR and next fix the index.

pkubatrh · 2018-03-13T11:46:47Z

[test-openshift]

praiskup · 2018-03-13T12:28:40Z

@hhorak would it be OK to have the cccp outage temporarily?

pkubatrh · 2018-03-14T10:08:52Z

Since Honza will not be able to comment on this for a while and everyone else is in favour of merging the PR and fixing the issues (and the tests are green) let us merge this in.

TomasTomecek · 2018-03-14T11:32:25Z

sclorg/postgresql-container#203 Sources needs to be generated before build

pkubatrh reviewed Nov 2, 2017

View reviewed changes

dhodovsk force-pushed the ultimate-distgen branch 2 times, most recently from cd87eff to 117f913 Compare November 3, 2017 09:56

pkubatrh reviewed Nov 3, 2017

View reviewed changes

dhodovsk force-pushed the ultimate-distgen branch from 117f913 to 49aa1dd Compare November 6, 2017 07:28

praiskup reviewed Nov 9, 2017

View reviewed changes

pkubatrh mentioned this pull request Nov 21, 2017

templates: use pg_isready as liveness probe #206

Merged

dhodovsk mentioned this pull request Nov 21, 2017

Distgen - support for image generation sclorg/container-common-scripts#38

Merged

dhodovsk force-pushed the ultimate-distgen branch from 49aa1dd to ecd0366 Compare December 8, 2017 14:48

praiskup reviewed Jan 4, 2018

View reviewed changes

dhodovsk force-pushed the ultimate-distgen branch 3 times, most recently from 2352783 to b50df2c Compare January 8, 2018 16:15

dhodovsk added 6 commits March 12, 2018 13:11

Create src directory from latest (step one)

9ac9395

Create src directory from latest (step two)

76086f2

History for Dockerfile.template (step one)

5a1e9f8

History for Dockerfile.template (step two)

15f5e74

Add manifest for generation and templates

a18a24a

Drop version 9.2

44c5339

dhodovsk force-pushed the ultimate-distgen branch from cef8c00 to 8d2bc73 Compare March 12, 2018 12:32

Drop files that can be generated

32925c9

dhodovsk force-pushed the ultimate-distgen branch from 8d2bc73 to 32925c9 Compare March 12, 2018 12:36

pkubatrh reviewed Mar 13, 2018

View reviewed changes

dhodovsk force-pushed the ultimate-distgen branch from 83946f1 to 32925c9 Compare March 13, 2018 08:25

add distgen prebuild hook

abbb5c5

pkubatrh merged commit 7032ddb into sclorg:master Mar 14, 2018

pkubatrh mentioned this pull request Mar 14, 2018

Add pre-build hook for sclorg postgresql CentOS/container-index#311

Merged

omron93 pushed a commit to omron93/container-index that referenced this pull request Mar 15, 2018

Fix centos/postgresql-XX-centos7 images after

91d7ec8

sclorg/postgresql-container#203 Sources needs to be generated before build

pkubatrh mentioned this pull request Mar 23, 2018

404 on documentation #262

Closed

More distgen - directory generation #203

More distgen - directory generation #203

Conversation

dhodovsk commented Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

praiskup Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkubatrh commented Nov 2, 2017

pkubatrh commented Nov 2, 2017

pkubatrh commented Nov 2, 2017

dhodovsk commented Nov 3, 2017

Choose a reason for hiding this comment

pkubatrh commented Nov 3, 2017

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

dhodovsk commented Dec 14, 2017

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

praiskup commented Jan 4, 2018

phracek commented Jan 8, 2018

TomasTomecek commented Jan 8, 2018

dhodovsk commented Jan 9, 2018

dhodovsk commented Mar 12, 2018

TomasTomecek commented Mar 12, 2018

pkubatrh commented Mar 13, 2018

pkubatrh commented Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

pkubatrh commented Mar 13, 2018

dhodovsk commented Mar 13, 2018

pkubatrh commented Mar 13, 2018

dhodovsk commented Mar 13, 2018

pkubatrh commented Mar 13, 2018

omron93 commented Mar 13, 2018

pkubatrh commented Mar 13, 2018

praiskup commented Mar 13, 2018

pkubatrh commented Mar 14, 2018

TomasTomecek commented Mar 14, 2018

dhodovsk commented Nov 2, 2017 •

edited

Loading

praiskup Nov 2, 2017 •

edited

Loading

pkubatrh commented Mar 13, 2018 •

edited

Loading