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

updated to latest version #2

Merged
merged 3 commits into from Apr 25, 2012
Merged

updated to latest version #2

merged 3 commits into from Apr 25, 2012

Conversation

mente
Copy link
Contributor

@mente mente commented Apr 19, 2012

Hi,
Updated your code to my own needs. Maybe you find it useful for yourself as well.

Regards,
Alex

added list origin identities, create origin identities
fixed update distribution config
added signedUrl generation for canned policy
@tellnes
Copy link
Owner

tellnes commented Apr 19, 2012

This looks great. I have not had the need for private content yet, so I left out the bit when I wrote this.

I will probably not have time to look at it until Monday, but will try to merge and pushed it to NPM then.

@mente
Copy link
Contributor Author

mente commented Apr 23, 2012

Looking forward for your feedback and 0.1.2 version ;)

Enabled: config.enabled,
DefaultRootObject: config.defaultRootObject
Comment: config.comment || '',
Enabled: config.enabled
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it should have been !!config.enabled so we are certain that it is a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can change it, if you like

Copy link
Owner

Choose a reason for hiding this comment

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

Probably I'll do it after merging.

@tellnes
Copy link
Owner

tellnes commented Apr 24, 2012

I see you've fixed some bugs that I have overlooked. Self I use it only to retrieve information from the CloudFront, as thus is the put / post / delete functionality untested. It should perhaps have been written a test suite.

I see you also have some indentation errors. I use two spaces.

I look forward to your responses to my comments as well :)

removed url dependency
use 2 spaces indentation
@mente
Copy link
Contributor Author

mente commented Apr 24, 2012

Fixed indentation overall the file, removed console.log, removed url module usage

CallerReference: callerReference,
Comment: comment || ''
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

Missing semicolon.

@tellnes
Copy link
Owner

tellnes commented Apr 24, 2012

Do you use an automatic tool to fix the indentation? You have edited quite a bit.

@mente
Copy link
Contributor Author

mente commented Apr 25, 2012

Yes, used TextMate

tellnes added a commit that referenced this pull request Apr 25, 2012
support private contents
@tellnes tellnes merged commit 92af4d4 into tellnes:master Apr 25, 2012
@tellnes
Copy link
Owner

tellnes commented Apr 25, 2012

I've added a getPrivateUrl method. The idea is that it should support all possible forms of signing. It works pretty much like the get_private_object_url method in the PHP SDK. See examples/getPrivateUrl.js and the comments before the implementation to see how it works.

Since this method implements everything that signUrlWithCannedPolicy support, do I not see the need for signUrlWithCannedPolicy anymore.

I have also implemented the rest of the OAI methods (PUT and DELETE). This I have done a bit differently than the way it is done in the rest of the lib. Please take a look at examples/setOAIComment.js and examples/deleteOAI.js for details. I am considering doing so for all PUT and DELETE methods since you must get hold of the etag before you can perform them anyways.

@mente
Copy link
Contributor Author

mente commented Apr 26, 2012

Looks good, but I think there're too much ways to parse getPrivateUrl :). Did you push changes to npm?

@tellnes
Copy link
Owner

tellnes commented Apr 26, 2012

Probably a little many, but I imagine just to document a few of them. I push it to npm today.

CloudFrontOriginAccessIdentity#delete should probably be renamed since delete is a reserved word. Some suggestions?

@mente
Copy link
Contributor Author

mente commented Apr 27, 2012

I think deleteIdentitiy would be enough

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.

None yet

2 participants