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 CLI params for disabling deleting and config/policy/cors updates #28

Closed
wants to merge 1 commit into from

Conversation

orm-forks
Copy link

We have added a bit more flexibility in what the plugin does to the S3 bucket as part of the flow.

@amsross
Copy link
Contributor

amsross commented Feb 28, 2018

I personally love this. Could you add some more information to the README, though? Also, spacing issues.

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.

Overall seems like some pretty good additions. A few things to change and we need to consider if we want these as --flags or config values. I imagine you'd want to keep these as flags to support running the deploy differently at runtime.

@@ -46,7 +46,7 @@ echo "error page" >> client/dist/error.html
**Fourth**, run the plugin, and visit your new website!

```
serverless client deploy [--stage $STAGE] [--region $REGION]
serverless client deploy [--stage $STAGE] [--region $REGION] [--no-delete-contents] [--no-config-change] [--no-policy-change] [--no-cors-change]
Copy link
Owner

Choose a reason for hiding this comment

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

Does the no in all the flags get removed somewhere?

These options make sense to me. Just have to figure out how to refactor some of the documentation to explain these options. And also decide if these make sense as command line args or as config details in serverless.yml.

@@ -31,6 +31,7 @@ class Client {
this.serverless = serverless;
this.provider = 'aws';
this.aws = this.serverless.getProvider(this.provider);
this.options = options;
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 remove the tabs and add in spaces everywhere? You can review in GitHub to compare the spacing here.

@fernando-mc
Copy link
Owner

When you get around to making an update to this can you also please bring in the style fixes and recent changes to resolve conflicts?

@linusmarco
Copy link
Contributor

@orm-forks, if you have any questions or need any help merging the recent linting/docs changes into this PR, just let me know.

@tim-stasse
Copy link

+1

This would be excellent to have!

Can someone else please fix this if the author doesn't do it soon?

@linusmarco
Copy link
Contributor

I'm working on a major rewrite of the library that should be done soon. I can probably incorporate this into that PR

linusmarco added a commit to linusmarco/serverless-finch that referenced this pull request Apr 7, 2018
@linusmarco linusmarco mentioned this pull request Apr 7, 2018
fernando-mc pushed a commit that referenced this pull request Apr 21, 2018
* split up code more logically, get bucket removal working

* finish up refactor

* more refactoring

* split out configure functions into separate file

* add object metadata functionality back in

* fix typo in README

* start log of changes for v2.0.0

* re-allow command-line region option, update log messages

* adding CLI params for disabling deleting and config/policy/cors updates (resolves #28 and resolves #15)

* add routing options in

* finish off validation for all configuration options

* update changelog

* update changelog again and add myself as maintainer

* addressing fernando's code review comments

* comment out all tests for now -- will be rewritten

* add confirmation prompts on deploy and remove
@linusmarco
Copy link
Contributor

@tim-stasse This is now live in v2.0.0!

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.

6 participants