-
Notifications
You must be signed in to change notification settings - Fork 77
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
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.
|
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.
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 |
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 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
Outdated
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
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.
done
@frankyn @dmitry-fa implemented in the same manner as other methods updating Bucket's properties. PTAL when you get chance |
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.
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 |
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.
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