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 a keyPrefix option #76

Merged
merged 3 commits into from
Mar 19, 2019
Merged

Add a keyPrefix option #76

merged 3 commits into from
Mar 19, 2019

Conversation

Archanium
Copy link
Contributor

Background

I've got a need for the keyPrefix mentioned in #73, so this is a little attempt at adding that. :)

Proposed changes

Adding a keyPrefix option, so that it's possibly to upload files to a prefixed s3 path.

Proposed reviewers (optional)

[@ mentions of other contributors]

@fernando-mc
Copy link
Owner

@Archanium this looks pretty cool. Can you do me a favor and rebase and test it again? Then I can bring it into a new version.

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! Can you also please add a section to the README for usage? Then I can merge it and cut a new release @Archanium

);
if (!keyPrefix) {
this.serverless.cli.log(
`Success! Your files have been removed and your bucket has been deleted`
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a logging statement here for if(keyPrefix)

Let's give the user clarification that files from the keyPrefix are deleted.

@fernando-mc
Copy link
Owner

@Archanium Actually, don't worry about it. I should have asked you to do that before. I'll merge now and add that myself with the new release.

@fernando-mc fernando-mc merged commit f8aabed into fernando-mc:master Mar 19, 2019
@fernando-mc fernando-mc mentioned this pull request Mar 19, 2019
1 task
@fernando-mc
Copy link
Owner

Just pushed this. Should be out soon. Had to fix an issue that came up when keyPrefix was undefined but seems to work now. Thanks again!

@Archanium
Copy link
Contributor Author

You're very welcome! :) Thanks for the rest of it!

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