-
Notifications
You must be signed in to change notification settings - Fork 4
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
Resolve bucket name dynamicly #3
base: master
Are you sure you want to change the base?
Conversation
@coyoteecd or @f00f any feedback? |
Hi @jettary I'm flattered by your choosing me as reviewer ;-) |
@@ -45,6 +45,9 @@ custom: | |||
# (optional) Buckets to remove before a stack is deployed. | |||
bucketsToCleanOnDeploy: | |||
- oldBucketName | |||
|
|||
# (optional) Get deployment bucket name from AWS. Works only for `sls s3remove` and `sls remove` | |||
autoResolve: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin is not meant to clean just the Serverless deployment buckets, think of stacks that create additional S3 buckets for other purposes, such as web apps. I think autoResolve
in this context is a bit too open-ended, so I'd suggest a more targeted name. E.g. cleanDeploymentBucket
, includeDeploymentBucket
, or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@f00f sorry for mentioning ) just because you are appears in "maintainers"
@coyoteecd good point. For renaming option.
@@ -67,6 +68,10 @@ export default class ServerlessS3Cleaner implements Plugin { | |||
const config = this.loadConfig(); | |||
let bucketsToEmpty = isDeploying ? config.bucketsToCleanOnDeploy : config.buckets; | |||
|
|||
if (!isDeploying && config.autoResolve) { | |||
bucketsToEmpty.push(await this.provider.getServerlessDeploymentBucketName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work properly with an explicit bucket name as well as a generated (default) bucket name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind logic is to delete deployment bucket only on remove
operation according with the rest of listed buckets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request Serverless Deployment Bucket Name created during previous deploys. No needs to know exact deployment buckeet.