search

drone-plugins / drone-s3-sync

Drone plugin for syncing artifacts with an Amazon S3 Bucket
http://plugins.drone.io/drone-plugins/drone-s3-sync
Apache License 2.0
24 stars 32 forks source link

Cloudfront #11

Closed ericanderson closed 8 years ago

ericanderson commented 8 years ago

Fixes #10

bradrydzewski commented 8 years ago

@nlf do you mind taking a look?

nlf commented 8 years ago

i'll check it in an hour or so :)

nlf commented 8 years ago

apparently "an hour or so" meant tomorrow, sorry about that.

LGTM

jackspirou commented 8 years ago

LGTM

jackspirou commented 8 years ago

I think everything is good functionally, but this is the first time I am seeing something like:

a := newApp()
if err := a.loadVargs(); err != nil {...}
if err := a.sanitizeInputs(); err != nil {...}

I don't think this is a common pattern in the other plugins.

nlf commented 8 years ago

it's not a common pattern in other plugins, no. but it does enhance testability so i wasn't going to object to it. we don't have a plugin style guide currently (that i'm aware of) so i was going off the idea "if it's still readable, it's good enough"

jackspirou commented 8 years ago

@nlf I think that is fair and valid, I just wanted to mention it. And I think a style guide would be great for plugins, just as long as we don't completely stop progress for the sake of style (like you inferred above).

On that note I will merge :)