Fix broken oci-image-tool #177

runcom · 2016-07-22T09:28:45Z

$ mkdir fedora-oci
$ skopeo copy docker://fedora oci:fedora-oci
$ mkdir fedora-bundle
$ oci-image-tool create-runtime-bundle --ref latest fedora-oci fedora-bundle
# get most of the errors fixed in this PR

Eventually the extraction fails because of permissions but this is expected because fedora has /root populated while for instance busybox doesn't have it so the only option is to unpack as root:

$ oci-image-tool create-runtime-bundle --ref latest fedora-oci fedora-bundle
[...]
unpacking failed: error extracting layer: unable to open file: open fedora-bundle/rootfs/root/.bash_logout: permission denied

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2016-07-22T09:41:56Z

ping @s-urbaniak

runcom · 2016-07-22T09:44:28Z

also the tar extraction is copied from docker createTarFile used in Unpack in pkg/archive why don't we use that instead?

runcom · 2016-07-22T10:11:13Z

funny go lint failure...

s-urbaniak · 2016-07-22T11:09:57Z

image/config.go

 func findConfig(w walker, d *descriptor) (*config, error) {
 	var c config
-	cpath := filepath.Join("blobs", d.Digest)
+	cpath := filepath.Join("blobs", d.getDigest())


let's not do getters

s-urbaniak · 2016-07-22T11:15:10Z

Just a couple of nits, thanks 👍 back then I didn't have a reference tool to build those images.

runcom · 2016-07-22T11:24:00Z

yet, the fedora image doesn't unpack correctly - should we just use the Docker's Unpack from pkg/archive instead of this copy-pasted fork which isn't really working?

s-urbaniak · 2016-07-22T11:27:41Z

@runcom sure, if it helps

runcom · 2016-07-22T11:30:53Z

I won't hold this PR though - the fedora image fails with permission denied because its /root is populated while, for instance, busybox does not populate it. I think this is good to go for now.

s-urbaniak · 2016-07-22T11:34:41Z

lgtm, once green

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2016-07-22T14:02:55Z

image/manifest.go:130::warning: cyclomatic complexity 31 of function unpackLayer() is high (> 20) (gocyclo)

can we relax gocyclo? seems like that function is that complex because it needs to be (it's the same in Docker)

wking · 2016-07-22T14:11:27Z

image/descriptor.go


+func (d *descriptor) normalizeDigest() string {
+	return strings.Replace(d.Digest, ":", "-", -1)
+}


This is a dup of #165, and I'd still rather fix it by getting #159 landed ;).

I see, but oci-image-tool isn't working at all w/o this fix and your #159 it's a rather bigger PR with a broader scope, I'd love to have this merged so people can use this tool instead of waiting on #159

On Fri, Jul 22, 2016 at 07:18:32AM -0700, Antonio Murdaca wrote:

+func (d *descriptor) normalizeDigest() string {

return strings.Replace(d.Digest, ":", "-", -1)
+}

I see, but oci-image-tool isn't working at all w/o this fix and
your #159 it's a rather bigger PR with a broader scope, I'd love to
have this merged so people can use this tool instead of waiting on
#159

I'm not sure what's holding #159 up, it's been out there for a while
;). So I'm not sure what timeframe to expect before people can use it
(although folks are certainly free to use it now, without waiting on
the branch to land in this repo ;).

I'm not sure what's holding #159 up, it's been out there for a while
;). So I'm not sure what timeframe to expect before people can use it
(although folks are certainly free to use it now, without waiting on
the branch to land in this repo ;).

I don't know either - just that you PR is 1000+ loc changed - while this one is just pulling in 10 additions (0e8f74b) to fix a fatal bug.

I believe this should go in asap. Potentially #159 can take a lot more time to be reviewed (not maintainers reviewed it yet)

On Fri, Jul 22, 2016 at 07:52:32AM -0700, Antonio Murdaca wrote:

I believe this should go in asap. Potentially #159 can take a lot
more time to be reviewed (not maintainers reviewed it yet)

I think it would be more efficient to put better abstractions into
place as soon as possible, instead of iterating on an approach we
expect to replace. But that's a throughput vs. latency issue for the
maintainers to figure out ;).

the point now is that, unfortunately this tool is broken and it's broken since a lot now probably (#165 is 28 days old why was that closed)
Anyhow I'm ok either way...

On Fri, Jul 22, 2016 at 08:09:47AM -0700, Antonio Murdaca wrote:

the point now is that, unfortunately this tool is broken and it's
broken since a lot now…

It's been broken (on this semicolon point) since it was born, because
the implementation was merged in #82 well before the spec landed in
#94. That means that nobody has ever used it for spec-compliant
unpacking, which takes the pressure off (in my opinion) for fixing it
ASAP.

s-urbaniak · 2016-07-22T14:22:04Z

@runcom sure, do you mind adding an additional commit, and bump --cyclo-over=20 in .tool/lint?

runcom · 2016-07-22T14:23:09Z

@runcom sure, do you mind adding an additional commit, and bump --cyclo-over=20 in .tool/lint?

done, thanks @s-urbaniak

Signed-off-by: Antonio Murdaca <[email protected]>

vbatts · 2016-07-25T18:51:48Z

LGTM

philips · 2016-07-29T16:11:58Z

LGTM

runcom added 3 commits July 22, 2016 11:14

cmd/oci-image-tool: replace colon with hyphen to get blobs by digest

0e8f74b

Signed-off-by: Antonio Murdaca <[email protected]>

cmd/oci-image-tool: fix config.json generation

116950e

Signed-off-by: Antonio Murdaca <[email protected]>

cmd/oci-image-tool: error out if path destination does not exist

6541392

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fix-oci-image-tool branch from 29dcda5 to 42d14df Compare July 22, 2016 11:00

s-urbaniak reviewed Jul 22, 2016
View reviewed changes

runcom force-pushed the fix-oci-image-tool branch from 42d14df to 36bf5e0 Compare July 22, 2016 11:22

runcom force-pushed the fix-oci-image-tool branch from 36bf5e0 to 5c9fb98 Compare July 22, 2016 13:30

cmd/oci-image-tool: fix unpacking...

44210d0

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fix-oci-image-tool branch from 5c9fb98 to 44210d0 Compare July 22, 2016 14:02

wking reviewed Jul 22, 2016
View reviewed changes

.tool/lint: bump gocyclo req

08bed93

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fix-oci-image-tool branch from 7647d0b to 08bed93 Compare July 22, 2016 14:43

philips merged commit 0bac884 into opencontainers:master Jul 29, 2016

runcom deleted the fix-oci-image-tool branch July 29, 2016 16:58

This was referenced Aug 10, 2016

ChangeLog: add the v0.4.0 release notes #195

Merged

image: Refactor to use cas/ref engines instead of walkers #159

Closed

wking mentioned this pull request Oct 18, 2016

image/manifest: Recursively remove pre-existing entries when unpacking opencontainers/image-tools#42

Merged

Fix broken oci-image-tool #177

Fix broken oci-image-tool #177

Uh oh!

Conversation

runcom commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

runcom commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Jul 22, 2016

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

s-urbaniak commented Jul 22, 2016

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

s-urbaniak commented Jul 22, 2016

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Jul 22, 2016

Uh oh!

runcom commented Jul 22, 2016

Uh oh!

vbatts commented Jul 25, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philips commented Jul 29, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

runcom commented Jul 22, 2016 •

edited

Loading

runcom commented Jul 22, 2016 •

edited

Loading

vbatts commented Jul 25, 2016 •

edited by caniszczyk

Loading

philips commented Jul 29, 2016 •

edited by caniszczyk

Loading