sphereio / node-s3-utils

A Command Line Interface providing some utilities for managing AWS S3 resources
MIT License
7 stars 2 forks source link

Feature/expiration headers #23

Closed dariuszor closed 8 years ago

dariuszor commented 8 years ago

@butenkor could you have a look please?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 85.6% when pulling d8b6c48d605903c4d3b77ff0f79aced911120696 on dariuszor:feature/expiration_headers into 2f227a4074aa9fa0b78f5dc5a3cde2723a9347c0 on sphereio:master.

dariuszor commented 8 years ago

@hajoeichler Hi, could you take a look? It's pretty urgent for merkur. Also: should I bump a version?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 85.6% when pulling 20f8d6e490e1b16e55b72cda6fd995ae0824a8f6 on dariuszor:feature/expiration_headers into 2f227a4074aa9fa0b78f5dc5a3cde2723a9347c0 on sphereio:master.

dariuszor commented 8 years ago

@onibox, @anas-aso, maybe you could have a look?

hajoeichler commented 8 years ago

A general question. Doesn't it make sense to always set a cache header, even if not provided the tool defines the default value.

dariuszor commented 8 years ago

@hajoeichler yes, it makes sense. Which value should I use as a default? 30 days?

svenmueller commented 8 years ago

@dariuszor please see my inline comments

svenmueller commented 8 years ago

Btw, we are already using this tooling in many projects, so the behavior should not change for them (eg. no expiration header, images are not compressed etc) if not configured.

dariuszor commented 8 years ago

@svenmueller thanks for detailed infos! I will move configuration to yaml file, update comments/README and remove default values if were not configured.

svenmueller commented 8 years ago

Thx a lot :) :+1:

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.06%) to 85.484% when pulling 12b03ab85028760c1c2a31744ca8e63a76d22d1c on dariuszor:feature/expiration_headers into 2f227a4074aa9fa0b78f5dc5a3cde2723a9347c0 on sphereio:master.

dariuszor commented 8 years ago

@svenmueller I've updated PR. I hope I didn't miss anything. I removed one test from previous commit. I didn't feel it test anything, and it hadn't influence on coverage either. headers = 'x-amz-acl': 'public-read' was implemented before, so I left it to not change configuration of other project.

svenmueller commented 8 years ago

@dariuszor LGTM

dariuszor commented 8 years ago

@svenmueller Great, thanks! Could you merge? I don't have write access.