Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Server Side Encryption #67

Merged
merged 5 commits into from
Oct 9, 2016
Merged

Conversation

sethpollack
Copy link
Contributor

What Changed & Why

Added an optional param serverSideEncryption.

PR Checklist

  • [x ] Add tests - just passed them.
  • [x ] Add documentation

@LevelbossMike
Copy link
Member

LevelbossMike commented Oct 3, 2016

I just restarted the Travis build and this LGTM. I'll merge this as soon as the build passes (one small nitpick change requested). Just out of curiosity what's the reason behind using server side encryption for index.html files? Company policy or real safety reasons?

@LevelbossMike
Copy link
Member

LevelbossMike commented Oct 3, 2016

@sethpollack as the documentation states that there are only two supported values for the serverside-encryption key and we are asserting on passing undefined as a value (which seems to be unsupported as a value for the encryption key) I would actually be more comfortable as a safety measure to see the tests be changed and check if the param is filtered out when the user did not specify the serverSideEncryption option.

Did you check that the plugin still functions as expected without this option present for users that don't need this? (which it should do according to the code changes just asking)

@@ -55,6 +56,10 @@ module.exports = CoreObject.extend({
CacheControl: cacheControl
};

if (serverSideEncryption) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please add a test for this conditional

@sethpollack
Copy link
Contributor Author

Just a company policy. Do these changes help?

@LevelbossMike
Copy link
Member

LevelbossMike commented Oct 5, 2016

@sethpollack tests are failing. I suggest we add two tests to the s3 client to make sure we are only adding this when the serverSideEncryption option is passed (one for not passing the option, one for passing). And the same to index-nodetest.

@sethpollack
Copy link
Contributor Author

Oh oops. Git added it back to the options. This should fix it.

I'll add the tests now.

@sethpollack
Copy link
Contributor Author

Actually, I need to add this to the activate hook too.

Will push up the changes soon.

@LevelbossMike LevelbossMike merged commit 9896c6c into ember-cli-deploy:master Oct 9, 2016
@LevelbossMike
Copy link
Member

🎉 thank you so much for this contribution @sethpollack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants