[merged] compose: support adding external files #253

giuseppe · 2016-03-30T15:02:19Z

This will allow to copy arbitrary files into the rootfs, specifying something like:

"add-files": [["service.template", "/exports/service.template"],
["config.json.template", "/exports/config.json.template"]]

It is quite useful when building a container image.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

cgwalters · 2016-03-31T13:17:34Z

#96 is related here.

But there's multiples reasons I've hesitated on this until now. It's an obvious feature but the thing is, as soon as it's introduced, it breaks the default "skip if unchanged" logic we have now which understands just RPMs.

It means that anyone doing a build system now needs to know about both RPMs and files (presumably stored in git).

We'd then need to separate out the logic for "detect if RPM set is unchanged" from "detect if files are changed".

Or alternatively we could add the external files into the checksum? Or another approach - automatically synthesize a noarch RPM from them?

cgwalters · 2016-03-31T14:01:57Z

I've really struggled with this problem...convenience (aka Dockerfile) is nice, but accuracy and efficiency are important too.

Particularly since for the use case you're thinking of is Docker format, we could maybe merge together ostree export with another tarball? Something like:

rpm-ostree compose tree --touch-if-changed=changed.stamp foo.json
if test -f changed.stamp; then
  return
fi
ostree export mycontainer | tar -Af - config.template service.template | docker load

?

cgwalters · 2016-03-31T14:40:42Z

We already had the checksum/check-if-changed problem with postprocess-script btw.

giuseppe · 2016-04-01T10:20:32Z

Even if it can be used by Docker, my idea is to skip Docker completely and use directly an ostree commit for storing the container image.
I thought about creating a rpm with the additional files but wouldn't it make the .json file less clear?

My other doubt about having add-files is: how will this co-exists with the new container command? Should something like this end up in container assemble?

giuseppe · 2016-04-04T13:05:29Z

actually not, files under /exports will be deleted even if I add them through a .rpm: #233

I have added a new patch, that takes added-files into account for the cache.

Would something like this work?

cgwalters · 2016-04-04T14:30:41Z

src/app/rpmostree-compose-builtin-tree.c

+
+          {
+            g_autofree guchar *ret_csum = NULL;
+            if (!ostree_checksum_file (srcfile,


See my comment in ostreedev/ostree#219 (comment) - this will also checksum uid/gid/xattrs, which is a bit weird since we'll be checksumming e.g. security.selinux on the host system, but on target may have a different one.

This does raise a general question about what the uid/gid for copied files should be. If for example rpm-ostree starts supporting non-root operation, we'd still want the target files to be owned by UID 0, not whatever UID was used to build the system.

cgwalters · 2016-04-04T14:59:41Z

I hadn't seen this updated PR before my comments in our standup today. It is looking like a good start. I don't want to block this, and it looks like it won't conflict too much with @jlebon 's work.

That said we don't have any tests yet either, and I'm worried about adding more features without those. I'll work some more on that.

jlebon · 2016-04-04T15:02:57Z

Definitely shouldn't be an issue to rebase pkg-layering on this.

giuseppe · 2016-04-04T15:03:46Z

I can add tests. I have not done it yet as this version was still a RFC

giuseppe · 2016-04-05T08:21:44Z

I added a fixup ⬆️ to use only the file content for the checksum. I added some tests as well

giuseppe · 2016-04-05T21:55:54Z

tests fixed now

cgwalters · 2016-04-07T20:36:49Z

Can you have the test also use --touch-if-changed and exercise the checksums?

cgwalters · 2016-04-07T20:37:11Z

tests/compose/test-repo-add-files.json

+
+    "repos": ["test-repo"],
+
+    "bootstrap_packages": [],


I'd just delete this now.

giuseppe · 2016-04-08T14:02:14Z

I have added --touch-if-changed to the tests. How would you like to test the checksums? Modify the code to break if an environment variable is set?

cgwalters · 2016-04-08T14:11:02Z

I meant that we change one of the external files, verify that results in a new commit. And conversely that not changing the files shouldn't result in a new commit.

giuseppe · 2016-04-08T14:26:03Z

I have changed the patch to also test the file changed case ⬆️

cgwalters · 2016-04-11T16:08:06Z

Forgotten git add?

make[2]: *** No rule to make target 'tests/compose/exported_file', needed by 'all-am'.  Stop.

giuseppe · 2016-04-11T19:45:49Z

ops I forgot to git add the Makefile-tests.am file. Done now.

cgwalters · 2016-04-27T13:53:54Z

Unfortunately fixup! fixup! doesn't work, can you rebase/squash?

This will allow to copy arbitrary files into the rootfs, specifying something like: "add-files": [["service.template", "/exports/service.template"], ["config.json.template", "/exports/config.json.template"]] It is quite useful when building a container image. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

cgwalters · 2016-04-27T17:31:17Z

I still have a hesitation about this regarding overall philosophy (having components come purely as RPM vs Dockerfile like whatever you want), but it does have a high enough utility/size ratio that I think it's worth adding. Thanks for the patch!

@cgwalters-bot r+ 4ccea7b

cgwalters-bot · 2016-04-27T17:31:23Z

⌛ Testing commit 4ccea7b with merge caa4d27...

This will allow to copy arbitrary files into the rootfs, specifying something like: "add-files": [["service.template", "/exports/service.template"], ["config.json.template", "/exports/config.json.template"]] It is quite useful when building a container image. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot · 2016-04-27T17:34:49Z

💔 Test failed - status-atomicjenkins

cgwalters · 2016-04-27T17:42:34Z

This is rpm-software-management/libdnf#108

jlebon · 2016-04-27T20:21:08Z

Looks like it was merged, so it should probably work now.
@cgwalters-bot retry

cgwalters-bot · 2016-04-27T20:21:14Z

⌛ Testing commit 4ccea7b with merge 9f29b24...

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot · 2016-04-27T20:24:52Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 9f29b24 to master...

copumpkin · 2016-05-13T12:32:21Z

@giuseppe @cgwalters A couple of thoughts (I'm not yet running a version that has this feature, but I find it appealing):

Why "The source file must be in the same directory as the treefile."? First off, doesn't the current state of the code allow for .. in the source path? Second, I have at least a couple of use cases for wanting an absolute path there.
Can the file copy happen before postprocess-script? My current strategy with postprocess-script is to use makeself to make the script carry a bunch of files with it, but this feels a little less hacky.

giuseppe · 2016-05-20T13:23:47Z

that was made to work in a similar way to Docker which uses the current directory as the context for the build. I think it is cleaner that allowing files not in the tree to be copied, as the build can be (more) easily reproduced.
the issue with that is then to decide what files will be kept. For example I would not be able to copy anything under /exports, as it would be wiped out by rpmostree_prepare_rootfs_for_commit. Not sure what would be the best solution for this. Probably add a new directive pre-copy-files that copies files before directories are moved around by rpmostree_prepare_rootfs_for_commit

giuseppe force-pushed the giuseppe/copy-files branch from e7f2d48 to 1d8d0a1 Compare April 4, 2016 13:37

cgwalters reviewed Apr 4, 2016
View reviewed changes

giuseppe force-pushed the giuseppe/copy-files branch 2 times, most recently from d54d24e to d3ce708 Compare April 5, 2016 14:15

cgwalters reviewed Apr 7, 2016
View reviewed changes

tests/compose/test-repo-add-files.json

"repos": ["test-repo"],

"bootstrap_packages": [],

Copy link

Member

cgwalters Apr 7, 2016

Choose a reason for hiding this comment

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

I'd just delete this now.

giuseppe force-pushed the giuseppe/copy-files branch from 26c0850 to ef01415 Compare April 8, 2016 14:25

giuseppe force-pushed the giuseppe/copy-files branch from 17edb42 to b07663f Compare April 27, 2016 14:01

giuseppe added 5 commits April 27, 2016 16:02

compose: add cache for added files

efe5ca5

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

fixup! compose: add cache for added files

0d3134b

tests: add tests for compose+add files

6b99abd

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

fixup! tests: add tests for compose+add files

4ccea7b

giuseppe force-pushed the giuseppe/copy-files branch from b07663f to 4ccea7b Compare April 27, 2016 14:02

cgwalters-bot pushed a commit that referenced this pull request Apr 27, 2016

compose: add cache for added files

f530970

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot pushed a commit that referenced this pull request Apr 27, 2016

tests: add tests for compose+add files

caa4d27

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot pushed a commit that referenced this pull request Apr 27, 2016

compose: add cache for added files

91ba029

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot pushed a commit that referenced this pull request Apr 27, 2016

tests: add tests for compose+add files

9f29b24

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #253 Approved by: cgwalters

cgwalters-bot changed the title ~~compose: support adding external files~~ [merged] compose: support adding external files Apr 27, 2016

cgwalters-bot closed this in 94a2804 Apr 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[merged] compose: support adding external files #253

[merged] compose: support adding external files #253

giuseppe commented Mar 30, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

giuseppe commented Apr 1, 2016

giuseppe commented Apr 4, 2016

cgwalters Apr 4, 2016

cgwalters commented Apr 4, 2016

jlebon commented Apr 4, 2016

giuseppe commented Apr 4, 2016

giuseppe commented Apr 5, 2016

giuseppe commented Apr 5, 2016

cgwalters commented Apr 7, 2016

cgwalters Apr 7, 2016

giuseppe commented Apr 8, 2016

cgwalters commented Apr 8, 2016

giuseppe commented Apr 8, 2016

cgwalters commented Apr 11, 2016

giuseppe commented Apr 11, 2016

cgwalters commented Apr 27, 2016

cgwalters commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters commented Apr 27, 2016

jlebon commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

copumpkin commented May 13, 2016

giuseppe commented May 20, 2016

[merged] compose: support adding external files #253

[merged] compose: support adding external files #253

Conversation

giuseppe commented Mar 30, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

giuseppe commented Apr 1, 2016

giuseppe commented Apr 4, 2016

cgwalters Apr 4, 2016

Choose a reason for hiding this comment

cgwalters commented Apr 4, 2016

jlebon commented Apr 4, 2016

giuseppe commented Apr 4, 2016

giuseppe commented Apr 5, 2016

giuseppe commented Apr 5, 2016

cgwalters commented Apr 7, 2016

cgwalters Apr 7, 2016

Choose a reason for hiding this comment

giuseppe commented Apr 8, 2016

cgwalters commented Apr 8, 2016

giuseppe commented Apr 8, 2016

cgwalters commented Apr 11, 2016

giuseppe commented Apr 11, 2016

cgwalters commented Apr 27, 2016

cgwalters commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters commented Apr 27, 2016

jlebon commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

cgwalters-bot commented Apr 27, 2016

copumpkin commented May 13, 2016

giuseppe commented May 20, 2016