feat: delete bucket OLM rules#352
Conversation
Codecov Report
@@ Coverage Diff @@
## master #352 +/- ##
============================================
+ Coverage 62.73% 63.13% +0.40%
- Complexity 554 601 +47
============================================
Files 31 32 +1
Lines 5023 5078 +55
Branches 480 481 +1
============================================
+ Hits 3151 3206 +55
- Misses 1707 1708 +1
+ Partials 165 164 -1
Continue to review full report at Codecov.
|
dmitry-fa
left a comment
There was a problem hiding this comment.
List<LifecycleRule> deleteLifecycleRules(String bucket, LifecycleRule... rulesToDelete);
Neither Storage nor Bucket defines methods to deal with LifecycleRules. Adding a method to them to delete that rules doesn't look logical. Would it make sense to implement this functionality in the BucketInfo class next to List<? extends BucketInfo.LifecycleRule> getLifecycleRules() method?
cc: @frankyn
|
+1 to @dmitry-fa, it would be better to define this in the BlobInfo builder because that's how metadata is mutated. |
|
@frankyn @dmitry-fa PTAL |
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Show resolved
Hide resolved
|
@frankyn PTAL |
dmitry-fa
left a comment
There was a problem hiding this comment.
The new implementation is much better than the previous one. Could you fix a few nits, please.
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
Show resolved
Hide resolved
google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java
Show resolved
Hide resolved
google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Outdated
Show resolved
Hide resolved
frankyn
left a comment
There was a problem hiding this comment.
I think you're super close, but I may have been too short on my surface expectations. Provided an example of what I would expect.
| * boolean rulesDeleted = bucket.deleteLifecycleRules(); | ||
| * if (rulesDeleted) { | ||
| * // the lifecycle rules were deleted | ||
| * } |
There was a problem hiding this comment.
I'm expecting the following surface:
String bucketName = "my-unique-bucket";
Bucket bucket = storage.get(bucketName);
updatedBucketMetdata = bucket.toBuilder().deleteLifecycleRules().build().update();
if (updatedBucketMetadata.getLifecycleRules().size() == 0) {
// the lifecycle rules were deleted
}|
@frankyn @dmitry-fa implemented in the same manner as other methods updating Bucket's properties. PTAL when you get chance |
dmitry-fa
left a comment
There was a problem hiding this comment.
Overall looks good to me. A few nits to address just left.
google-cloud-storage/src/test/java/com/google/cloud/storage/BucketTest.java
Show resolved
Hide resolved
google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
Show resolved
Hide resolved
|
@dmitry-fa all the comments have been addressed PTAL |
dmitry-fa
left a comment
There was a problem hiding this comment.
Looks good to me know, thanks for addressing my comments.
🤖 I have created a release \*beep\* \*boop\* --- ## [1.110.0](https://www.github.com/googleapis/java-storage/compare/v1.109.1...v1.110.0) (2020-06-18) ### Features * delete bucket OLM rules ([#352](https://www.github.com/googleapis/java-storage/issues/352)) ([0a528c6](https://www.github.com/googleapis/java-storage/commit/0a528c6916f8b031916a4c6ecc96ce5e49ea99c7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Fixes #344