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

Fix defaultMimeType option binding #94

Merged

Conversation

kiwi-josh
Copy link

What Changed & Why

defaultMimeType field is ignored in the ember-cli-deploy options, as it isn't passed into the s3 options correctly. This pull request fixes this binding

Related issues

Here is the issue: #93

Copy link
Member

@achambers achambers left a comment

Choose a reason for hiding this comment

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

Thanks dude. Just left a comment for you to consider.

@@ -61,6 +61,7 @@ module.exports = {
var dotFolders = this.readConfig('dotFolders');
var serverSideEncryption = this.readConfig('serverSideEncryption');
var batchSize = this.readConfig('batchSize');
var defaultMimeType = this.readConfig('defaultMimeType');
Copy link
Member

Choose a reason for hiding this comment

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

Even though this is specified in the README we don't have an actual default for it in the defaultConfig above. Worth adding it there?

Copy link
Author

Choose a reason for hiding this comment

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

@achambers What do you think about adding the default as simply application/octet-stream? because I guess thats the only reasonable default I can think of. Have you got any other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

@wytlytningNZ Yep, exactly. That's what the README says it is. So, what I meant was, would you mind adding that default in the defaultConfig prop as a part of this PR?

Copy link
Author

Choose a reason for hiding this comment

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

@achambers sure thing. Just added it now.
Interestingly though, application/octet-stream is already set to the default by the mime module if no default is supplied, but I guess its better be to more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

but I guess its better be to more explicit.

Yep. The principle of least surprise and all that :)

@achambers achambers merged commit ba530e6 into ember-cli-deploy:master May 31, 2017
@achambers
Copy link
Member

Released as v1.1.0

Thanks @wytlytningNZ

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