Ensure its possible to remove stack with missing bucket
#10306
Conversation
pgrzesik
changed the title
async/await syntax in bucket.jsremove stack with missing bucket
Codecov Report
@@ Coverage Diff @@
## master #10306 +/- ##
=======================================
Coverage 85.41% 85.42%
=======================================
Files 339 340 +1
Lines 13906 13917 +11
=======================================
+ Hits 11878 11888 +10
- Misses 2028 2029 +1
Continue to review full report at Codecov.
|
pgrzesik
force-pushed
the
allow-for-removal-of-stack-with-removed-bucket
branch
from
dace405
to
3bacaf5
Compare
lib/plugins/aws/remove/lib/bucket.js
Outdated
| }); | ||
| const bucketName = await this.provider.getServerlessDeploymentBucketName(); | ||
| this.bucketName = bucketName; | ||
| this.checkIfBucketExists(bucketName); |
There was a problem hiding this comment.
What's the purpose of that call?
There was a problem hiding this comment.
While implementing the second PR I've discovered a bug in this implementation for a specific case and I've adjusted the implementation a little bit
There was a problem hiding this comment.
Ok, but we do not take result of that method. What is the reason of checking if it exists, if we're not interested in result?
There was a problem hiding this comment.
This code has been changed, please see the updated version
| .then(this.listObjects) | ||
| .then(this.deleteObjects); | ||
| await this.setServerlessDeploymentBucketName(); | ||
| if (this.bucketName && (await this.checkIfBucketExists(this.bucketName))) { |
There was a problem hiding this comment.
Wouldn't bucketName be unconditionally set here? (we didn't have that check before)
There was a problem hiding this comment.
There is a scenario in which it won't be set so we need to leave this check this way
There was a problem hiding this comment.
Is this a scenario that was already in master and was not handled? Or is this scenario introduced with this PR?
Also when exactly is that the case?
There was a problem hiding this comment.
Yes, in master it was just throwing an error on SDK call in setServerlessDeploymentBucket, now we want to handle such situation gracefully
pgrzesik
force-pushed
the
allow-for-removal-of-stack-with-removed-bucket
branch
from
3bacaf5
to
cf4a729
Compare
Addresses the
removepart of #3964I've also cleaned up the tests in the process because there were a few leftover tests in
bucket.test.jsAddressing
deployscenario will come in a separate PR