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

Give more instructions on bucket policy. #71

Merged
merged 3 commits into from Nov 19, 2018

Conversation

jrschumacher
Copy link
Contributor

Background

Since [some/path/here.json] translates to ["some/path/here.json"] it is confusing and I thought the policy required an array of strings.

Proposed changes

Update the README and us "some/path/policy.json" instead. Also added some tips about env paths since finch will try and look in serverless-finch module directory path for relative path policies.

Since `[some/path/here.json]` translates to `["some/path/here.json"]` it is confusing and I thought the policy required an array of strings.
@jrschumacher
Copy link
Contributor Author

@fernando-mc

@fernando-mc
Copy link
Owner

@jrschumacher thanks for the PR!

Can we keep the default as "/path/to/policy.json" but also make sure to add the additional example with ${env:PWD} in the notes?

Something like this maybe?

 _Note: You can also use `${env:PWD}` if you want to dynamically specify the policy within your repo. for example: 

bucketPolicyFile: "${env:PWD}/path/to/policy.json"

 Additionally, you will want to specify different policies depending on your stage using `${self:provider.stage}` to ensure your `BUCKET_NAME` corosponds to the stage._

This could also use an example.

Thanks for the suggestions!

@jrschumacher
Copy link
Contributor Author

Sure I'll make the change. Would you also accept a PR if I updated the README to avoid the use of brackets throughout (unless the option is an array)?

@fernando-mc
Copy link
Owner

@jrschumacher I think that would be fine and makes sense.

Please keep them in cases like this:
serverless client deploy [--region $REGION] [--no-delete-contents]

Otherwise I think you're correct and that they imply an array.

Nice catch!

@jrschumacher
Copy link
Contributor Author

@fernando-mc thoughts?

Copy link
Owner

@fernando-mc fernando-mc left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the improvement!

@fernando-mc fernando-mc merged commit 0f29c4c into fernando-mc:master Nov 19, 2018
@jrschumacher jrschumacher deleted the patch-1 branch November 24, 2018 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants