Feature/contextsources

[chrisz100]

Implementing a modular build context retrieval.
This will allow to define various sources, be it file, gs, s3 or any other.

Feel free to review on a commit-basis as this might be a bigger thing and correcting it all at once would be double the effort in the end.

[AkihiroSuda]

WDYT about using rclone?
It supports Amazon Drive, Amazon S3, Backblaze B2, Box, Ceph, DigitalOcean Spaces, Dreamhost, Dropbox, FTP, Google Cloud Storage, Google Drive, HTTP, Hubic, IBM COS S3, Memset Memstore, Microsoft Azure Blob Storage, Microsoft OneDrive, Minio, Nextloud, OVH, Openstack Swift, Oracle Cloud Storage, OwnCloud, pCloud, put.io, QingStor, Rackspace Cloud Files, SFTP, Wasabi, WebDAV, and Yandex Disk.

[chrisz100]

rclone seems to support a lot but wouldn’t that be a little much stuff to „just“ download and untar a file?

In any case we’d probably try to compile it into the executor as kaniko tries to operate on a almost scratch-like container.

Edit: If they allow it and MIT license is fine: I think importing the backends and using them for retrieving files would be a working solution. Plus they'd take care of keeping it compatible with the backends.

[chrisz100]

As we have a reader and can use this to process the unTar, when moving this to util replace the local storing with direct decompression.

[priyawadhwa]

It seems like using rclone would require the user to set up a special config rclone.conf, which I don't love the idea of.

I think it might make sense to create a buildcontext package with a BuildContext interface, which could look like this:

type BuildContext interface {
  // Gets context.tar.gz from the build context and unpacks to the directory
   UnpackTarFromBuildContext(buildContext string, directory string) error
}

This would make it easier to add build contexts in the future, similar to how we implemented the Docker commands. Then we could move stuff like this into that package.

We could also then get rid of the --bucket flag and keep only the --context flag, since GCS buckets would be identified as --context=gcs://bucket-name

[chrisz100]

Sounds good, I’ll put the interface into the changes.

I just put it all to root to get started so I planned to move the extract anyhow.

I’d also propose to move the part figuring out what build module to use to that package as this prefix parser is a little out of context there if we have a buildcontext package.

[dlorenc]

Sounds good, I’ll put the interface into the changes.

I just put it all to root to get started so I planned to move the extract anyhow.

I’d also propose to move the part figuring out what build module to use to that package as this prefix parser is a little out of context there if we have a buildcontext package.

Is this ready for another look?

[chrisz100]

Feel free to give your input! I’ve been blocked with other things so no changes there from my side I’m afraid. Will continue working on this this week.

[chrisz100]

@priyawadhwa @dlorenc can you have a quick look onto interface and the GC draft?

[priyawadhwa]

I think we should keep both flags so that this change is backwards compatible. Let's make sure that either
--context or --bucket is specified (and return an error if both or neither are), and pass whichever one is specified into GetBuildContext() (mentioned below)

Also, if we pass in --bucket with no prefix, assume a GCS bucket so that it's still backwards compatible :)

[priyawadhwa]

Thanks for the updates, I left some comments!

[priyawadhwa]

Could we move all of this logic into buildcontext.go? maybe a function like

func GetBuildContext(bucket string) (BuildContext, error)

It could also be nice to create a map in buildcontext.go to keep track of what prefixes correspond to which BuildContext, and go through the map to figure out which BuildContext to return.

var buildContextMap = map[string]*BuildContext {
   "gcs://" : &buildcontext.GCS{},
   ...
}

[chrisz100]

Moving it to a function is good. Just unsure what string function to use with a map for prefixes. Will look into that tomorrow.

[priyawadhwa]

I was thinking something like this:

func GetBuildContext(srcContext string) (*BuildContext, error) {
   for prefix, bc := range buildContextMap {
        if strings.HasPrefix(srcContext, prefix) {
              return bc, nil
        }
   }
   return nil, fmt.Error("invalid prefix")
}

[chrisz100]

When you look for a super fancy solution and it really is as simple as this.

[priyawadhwa]

nit: this should probably be GCS instead of GC

[chrisz100]

I think kops is using gc but I’ll check with the docs and such to make it match

[priyawadhwa]

I think we should keep both flags so that this change is backwards compatible. Let's make sure that either
--context or --bucket is specified (and return an error if both or neither are), and pass whichever one is specified into GetBuildContext() (mentioned below)

Also, if we pass in --bucket with no prefix, assume a GCS bucket so that it's still backwards compatible :)

[priyawadhwa]

nit: could we make this dir:// since there may be multiple files in a directory being used as the build context

[priyawadhwa]

Is this ready for review?

[chrisz100]

Let's see!

[chrisz100]

Almost forgot to add the s3 prefix for the s3 implementation again... That PR is getting way too big. Should be all stable now!

[priyawadhwa]

Hey @chrisz100, I wanted to refactor this a bit before merging and update the integration tests -- would you mind if I pushed a couple commits?

[chrisz100]

@priyawadhwa added you to the repository collaborators list. Feel free :-)

[priyawadhwa]

Thank you!

[chrisz100]

lgtm

[chrisz100]

Busy times as it seems. @priyawadhwa any update or outlook on getting this merged?

[priyawadhwa]

@chrisz100 thank you so much for your contribution! just merged :)