nienbo / cache-buildkite-plugin

Tarball, Rsync & S3 Cache Kit for Buildkite. Supports Linux, macOS and Windows
https://buildkite.com/plugins
MIT License
67 stars 39 forks source link

S3 args are passed to head-object call #35

Closed mogopz closed 3 years ago

mogopz commented 3 years ago

Hi, In your README you mention that the args argument gets passed to the s3 cp command but in the code it's also being passed to the aws s3api head-object command.

This breaks a lot of arguments that someone might want to potentially pass (in my case --acl) since the two commands don't share the same options. For example when I try to set --acl the cache plugin fails once it tries to check for an existing cache:

usage: aws [options] <command> <subcommand> [<subcommand> ...] [parameters]
--
  | To see help text, you can run:
  |  
  | aws help
  | aws <command> help
  | aws <command> <subcommand> help
  |  
  | Unknown options: --acl, bucket-owner-full-control

I'm happy to open a PR to fix this if you let me know how you'd prefer to handle this. The PR that added custom args to the head-object command mentions needing it so they can set the endpoint, but that's already configurable with the endpoint argument so I think it makes the most sense to just remove the custom args from the head-object call.

gencer commented 3 years ago

endpoint can be separately defined in yaml and it has a special key, however, it is still appended to $BK_AWS_ARGS which is piped to aws s3api head-object just like s3 cp command. See: https://github.com/gencer/cache-buildkite-plugin/blob/17ee0f78000acc59ef081aeda95bae38b75e10a3/lib/backends/s3.bash#L51

What we are going to do here is to use separate variable to hold pre-defined keys and only pass this variable to head-object and for other calls we will merge it with $BK_AWS_ARGS.

However, If you are intend to work on this PR, please take above into consideration.

mogopz commented 3 years ago

Thanks @gencer, I've opened a PR - let me know what you think

gencer commented 3 years ago

Thanks @mogggggg for this great PR. I've gone ahead and merged.

Will release a new version soon. 🚀🎉

gencer commented 3 years ago

Released as v2.4.9 🎉