New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unpack ability #75

Merged
merged 2 commits into from Jul 14, 2017

Conversation

Projects
None yet
5 participants
@krishicks
Contributor

krishicks commented Jun 29, 2017

When unpack is true and the file can be determined to be an archive it knows how to handle, the resource will unpack the archive to the destination directory after downloading it.

The way it works is by downloading the file the destination dir when the flag is set, and then if it's an archive it extracts it. If the archive is a .tar.gz it will unpack the tar after gunzipping it. The original archive is not saved.

This should mean that when the unpack option is used the unpacked archive is cached in baggageclaim, which can be a benefit when downloading large zip files that get unpacked multiple times by different jobs.

This should also mean that this resource can be used as an image_resource*, where your archive lays out the appopriate filesystem to be used for that case, which is having a root rootfs directory and metadata.json file as created by docker-image-resource after a get.

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jun 30, 2017

Contributor

This isn't quite ready; you can't use os.Rename across partitions; the directory has to be copied another way.

Contributor

krishicks commented Jun 30, 2017

This isn't quite ready; you can't use os.Rename across partitions; the directory has to be copied another way.

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 3, 2017

Contributor

This is ready now.

Contributor

krishicks commented Jul 3, 2017

This is ready now.

@ebabani ebabani requested review from vito and removed request for vito Jul 7, 2017

@chendrix chendrix added this to the v3.4.0 milestone Jul 10, 2017

@vito

I think this configuration makes more sense in params, so I opened concourse/concourse#1369 to support that. Would you mind amending this and then once we do the other issue it can be used for image_resource?

I'm also not a huge fan of recursing as there may be cases where you've got a .zip in a .tar.gz because...reasons. Automation can get weird. I'd prefer it to special case .tar.gz / .tgz.

Refactor some in_command tests and code
This will make adding the unpack feature easier to do.
@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 11, 2017

Contributor

I think params is a far more appropriate place for this.

Special-casing .tar.gz is also not a problem.

Contributor

krishicks commented Jul 11, 2017

I think params is a far more appropriate place for this.

Special-casing .tar.gz is also not a problem.

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 11, 2017

Contributor

Updated.

Contributor

krishicks commented Jul 11, 2017

Updated.

@vito

vito approved these changes Jul 12, 2017

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 12, 2017

Contributor

I'll amend the commit to have the correct message; it still says the option was added to source.

Contributor

krishicks commented Jul 12, 2017

I'll amend the commit to have the correct message; it still says the option was added to source.

Add unpack option to InRequest params
If the InRequest is configured with `Params{Unpack: true}`, unpack the archive
after downloading it. The original archive is not stored.

In other words, if you have an archive, `some-archive.tgz`, with a file
`some-file` in it, the destination directory after downloading and unpacking
will contain just `some-file`.
@andrewedstrom

This comment has been minimized.

Show comment
Hide comment
@andrewedstrom

andrewedstrom Jul 14, 2017

Now that we've finished concourse/concourse#1369, this should be ready to test and accept. We will support params in image_resources from 3.4 on.

andrewedstrom commented Jul 14, 2017

Now that we've finished concourse/concourse#1369, this should be ready to test and accept. We will support params in image_resources from 3.4 on.

@vito vito merged commit d772e4d into concourse:master Jul 14, 2017

1 check passed

ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details

@vito vito removed the workflow: discuss label Jul 14, 2017

@chendrix

This comment has been minimized.

Show comment
Hide comment
@chendrix

chendrix Jul 20, 2017

Contributor

This has been failing to build because bitbucket.org/taruti/mimemagic was not vendored in.

Contributor

chendrix commented Jul 20, 2017

This has been failing to build because bitbucket.org/taruti/mimemagic was not vendored in.

@chendrix

This comment has been minimized.

Show comment
Hide comment
@chendrix

chendrix Jul 20, 2017

Contributor

Actually, due to that package being potentially GPL, this PR is likely DOA.

Contributor

chendrix commented Jul 20, 2017

Actually, due to that package being potentially GPL, this PR is likely DOA.

chendrix added a commit that referenced this pull request Jul 20, 2017

Revert "Merge pull request #75 from krishicks/add-unpack-ability"
This reverts commit d772e4d, reversing
changes made to 887a453.

chendrix added a commit that referenced this pull request Jul 20, 2017

chendrix added a commit that referenced this pull request Jul 20, 2017

Use native net/http instead of custom package
#75

Signed-off-by: JT Archie <jarchie@pivotal.io>
@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 20, 2017

Contributor

I see two options:

  1. Find a replacement for mimemagic.
  2. Remove the auto-detection and require the user to say which utility to unpack with via params.
Contributor

krishicks commented Jul 20, 2017

I see two options:

  1. Find a replacement for mimemagic.
  2. Remove the auto-detection and require the user to say which utility to unpack with via params.
@chendrix

This comment has been minimized.

Show comment
Hide comment
@chendrix

chendrix Jul 20, 2017

Contributor

It looks like net/http supports this behavior automatically with http.DetectContentType

Contributor

chendrix commented Jul 20, 2017

It looks like net/http supports this behavior automatically with http.DetectContentType

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 20, 2017

Contributor

That's pretty awesome. Nice find!

Contributor

krishicks commented Jul 20, 2017

That's pretty awesome. Nice find!

@chendrix

This comment has been minimized.

Show comment
Hide comment
@chendrix

chendrix Jul 20, 2017

Contributor

Ok so I didn't see the reliance on gunzip unzip and tar binaries being available in the rootFS.

I'm really uncomfortable with this PR and would prefer if this feature were to be re-implemented, that it only use the standard library of Go, which has zip/tar/gzip support.

Contributor

chendrix commented Jul 20, 2017

Ok so I didn't see the reliance on gunzip unzip and tar binaries being available in the rootFS.

I'm really uncomfortable with this PR and would prefer if this feature were to be re-implemented, that it only use the standard library of Go, which has zip/tar/gzip support.

@krishicks

This comment has been minimized.

Show comment
Hide comment
@krishicks

krishicks Jul 20, 2017

Contributor

While the standard library does support all of those things, it is quite difficult to reproduce the same characteristics you get by default when using the binaries which exist on the rootfs. Un-tarring in particular is quite cumbersome.

The docker-image-resource relies on many binaries being present on the image, see common.sh:

  • mkdir
  • mountpoint
  • mount
  • sed
  • umount
  • ln
  • rm
  • grep
  • dockerd
  • kill
  • wait

I don't think it's unreasonable to rely on binaries that exist on the rootfs.

Contributor

krishicks commented Jul 20, 2017

While the standard library does support all of those things, it is quite difficult to reproduce the same characteristics you get by default when using the binaries which exist on the rootfs. Un-tarring in particular is quite cumbersome.

The docker-image-resource relies on many binaries being present on the image, see common.sh:

  • mkdir
  • mountpoint
  • mount
  • sed
  • umount
  • ln
  • rm
  • grep
  • dockerd
  • kill
  • wait

I don't think it's unreasonable to rely on binaries that exist on the rootfs.

@vito

This comment has been minimized.

Show comment
Hide comment
@vito

vito Jul 20, 2017

Member
Member

vito commented Jul 20, 2017

jtarchie added a commit that referenced this pull request Jul 21, 2017

find mimetypes for archive files
#75

Signed-off-by: Topher Bullock <cbullock@pivotal.io>

jtarchie added a commit to concourse/static-golang that referenced this pull request Jul 21, 2017

add unzip
concourse/s3-resource#75

Signed-off-by: Topher Bullock <cbullock@pivotal.io>

cappyzawa pushed a commit to cappyzawa/s3-resource that referenced this pull request Feb 5, 2018

cappyzawa pushed a commit to cappyzawa/s3-resource that referenced this pull request Feb 5, 2018

Revert "Merge pull request concourse#75 from krishicks/add-unpack-abi…
…lity"

This reverts commit d772e4d, reversing
changes made to 887a453.

cappyzawa pushed a commit to cappyzawa/s3-resource that referenced this pull request Feb 5, 2018

cappyzawa pushed a commit to cappyzawa/s3-resource that referenced this pull request Feb 5, 2018

Use native net/http instead of custom package
concourse#75

Signed-off-by: JT Archie <jarchie@pivotal.io>

cappyzawa pushed a commit to cappyzawa/s3-resource that referenced this pull request Feb 5, 2018

find mimetypes for archive files
concourse#75

Signed-off-by: Topher Bullock <cbullock@pivotal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment