Fix broken oci-image-tool

[runcom]

$ 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

[runcom]

ping @s-urbaniak

[runcom]

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

[runcom]

funny go lint failure...

[s-urbaniak]

let's not do getters

[s-urbaniak]

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

[runcom]

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]

@runcom sure, if it helps

[runcom]

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]

lgtm, once green

[runcom]

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]

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

[runcom]

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

[wking]

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 ;).

[runcom]

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)

[wking]

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 ;).

[runcom]

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...

[wking]

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]

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

[runcom]

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

done, thanks @s-urbaniak

[vbatts]

LGTM

[philips]

LGTM