Add destination prefix option to publish task

[johncmckim]

Adds a destination prefix option to the publish task to enable users to specify where artifacts are uploaded to on s3 more precisely. Resolves issue #10.

[johncmckim]

I had some issues running the tests.

java.lang.NoSuchMethodError: org.hamcrest.Matcher.describeMismatch

I had to change mockito-all to mockito-core in build.sbt to run them. Though I did not commit it as affected me before the changes and I wasn't sure if it was an environmental issue.

[manojlds]

@johncmckim apologies for not getting back sooner. Will review and merge in a couple of days.

[timanrebel]

@johncmckim I got it to post to the root of S3, thanks! but it throws an error when I set / as destinationPrefix:

20:00:36.601 Pushing /mnt/go-disk/go-agent/pipelines/styleguide/dist/style/style.css.map to s3://styleguide.peermatch.com/style/style.css.map
20:00:37.299 Pushed /mnt/go-disk/go-agent/pipelines/styleguide/dist/style/style.css.map to s3://styleguide.peermatch.com/style/style.css.map
20:00:37.992 Error: The request body terminated unexpectedly (Service: Amazon S3; Status Code: 400; Error Code: IncompleteBody; Request ID: CE57C2B56A8FD06B)
20:00:38.009 [go] Current job status: failed.

Do you have any idea? Interesting is that it succeeds in uploading, but fails to do something else. It might be setting the meta data. Can you reproduce this?
(it does not fail when I leave the destinationPrefix empty or set it to something like test)

[johncmckim]

@timanrebel I haven't seen that issue before. It does look like an issue with the setMetadata process. I haven't run into this issue when using my fork.

According to the docs that error occurs when the body "does not provide the number of bytes specified by the Content-Length HTTP header". According to the java sdk docs Content Length must be set before data is uploaded. The setMetadata method does this by setting a Content Length of 0 and sending an empty request .

In short I find that a really unusual error. Maybe @manojlds or @ashwanthkumar has seen this issue before?

[timanrebel]

@johncmckim is this the correct way to push to the root of an S3 bucket?

[timanrebel]

I think I understand what is going wrong here. The plugin tries to update the metadata of the bucket if destinationPrefix is /. Otherwise it updates the directory. The problem might be that AWS S3 does not allow metadata on a bucket (not unlogical)

This plugin is meant to publish artefacts, not so much deploy files to S3. Maybe we should create a fork that is optimised for deployments instead

[johncmckim]

@timanrebel sorry for the slow response, I've been looking into it a bit. Your theory does make sense. However, I can't find any information to confirm this.

I've got a task pushing to the root of an s3 bucket now too. I'm not getting the same error, I am getting a 403 when the plugin attempts to set the metadata. I can see why setting metadata is important when doing Artefact storage. However, I would think it less important on deployment.

I think you could have one plugin do both. I would rather have one plugin installed than two. However, at this stage since it has been 4 months since the original issue was created and there's an obvious need for this, perhaps a separate plugin could be a good idea. Before doing that, I'd really like to hear from one of the maintainers on this project, @manojlds @ashwanthkumar .

[ashwanthkumar]

I'm extremely sorry for the delay in getting back on this PR. @johncmckim You're right, you don't need to set metadata if you're going to use it only for deploying. We can choose not to set the metadata when deploying.

cc: @manojlds @Sriram-R

[ashwanthkumar]

If we could extract this, if check outside then we could choose to set / not set the metadata.

[johncmckim]

Done. I changed to to not set the metadata if a destinationPrefix is configured. I chose this instead of adding another setting as most using a destinationPrefix are deploying rather than storing artifacts.

[ashwanthkumar]

I guess this got imported by mistake :)

[ashwanthkumar]

This LGTM for merge. Any other suggestions @manojlds @Sriram-R ?

[brewkode]

Everyone - Apologies for not keeping a tab on this. This is good to be merged.

[ashwanthkumar]

I'm merging this now. Thanks @johncmckim for the contribution.