-
Notifications
You must be signed in to change notification settings - Fork 78
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: integrate disable Lifecycle rule api for to remove lifecycle rule of bucket #28
Conversation
This comment has been minimized.
This comment has been minimized.
Does this API remove all lifecycle rules? if so, it should be named disableLifecycleRules rather than rule, and if not there should be documentation on which rule specifically would be removed and how to control it |
@JesseLovelace yes,this API remove all lifecycle rules. so you are right we should named it as |
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.
Few notes:
- Let's avoid confusing/ambiguous language like "disabled or deleted". I say we go with "deleted," change all references to "disabled or deleted" to just say "deleted," and update the method to deleteLifecycleRules.
- It's Lifecycle, not LifeCycle
- Make sure to run the style checker even in commented out code to catch things like spaces after commas
@JesseLovelace PTAL |
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
Outdated
Show resolved
Hide resolved
Also, your IT is failing because you aren't using a real service account email |
Try using storage.getServiceAccount(projectId) instead of extracting it from the json like you're doing now |
done @JesseLovelace PTAL |
@JesseLovelace PTAL |
@frankyn @JesseLovelace gentle ping. |
@JesseLovelace is on point for this one. I'll bring it up in our meeting early next week (Tuesday). Thank you for your patience @athakor |
@frankyn @JesseLovelace here, how we can move ahead with above situation? |
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.
@@ -3208,4 +3209,36 @@ public void testBucketLogging() throws ExecutionException, InterruptedException | |||
RemoteStorageHelper.forceDelete(storage, loggingBucket, 5, TimeUnit.SECONDS); | |||
} | |||
} | |||
|
|||
@Test | |||
public void testDeleteLifecycleRules() throws ExecutionException, InterruptedException { |
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.
Following up, did you try deleting a single lifecycle rule?
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 have tried to find the feasible way to delete a single lifecycle rule but its look like there is no possible way to do that and also found that across all the languages have same behavior which we have currently implemented.
Suggestion
-
I think we should have to update the method name like
clearLifecycleRules
ordisableLifecycleRules
instead ofdeleteLifecycleRules
to be more clear,deleteLifecycleRules
might confused to user. -
we can also place a
Note
there like delete individual rules through the console because currently this library have limited support to delete rule.
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.
Hi @athakor, apologies for the delay.
String lifecycleTestBucketName = RemoteStorageHelper.generateBucketName();
storage.create(
BucketInfo.newBuilder(lifecycleTestBucketName)
.setLocation("us")
.setLifecycleRules(
ImmutableList.of(
new LifecycleRule(
LifecycleAction.newSetStorageClassAction(StorageClass.COLDLINE),
LifecycleCondition.newBuilder()
.setAge(1)
.setNumberOfNewerVersions(3)
.setIsLive(false)
.setCreatedBefore(new DateTime(System.currentTimeMillis()))
.setMatchesStorageClass(ImmutableList.of(StorageClass.COLDLINE))
.build()),
new LifecycleRule(
LifecycleAction.newDeleteAction(),
LifecycleCondition.newBuilder()
.setAge(1)
.build()
)))
.build());
// Delete OLM rule.
Bucket remoteBucket =
storage.get(lifecycleTestBucketName, Storage.BucketGetOption.fields(BucketField.LIFECYCLE));
int priorSize = remoteBucket.getLifecycleRules().size();
ArrayList<LifecycleRule> lifecycleRules = new ArrayList(remoteBucket.getLifecycleRules());
Iterator<LifecycleRule> iterator = lifecycleRules.iterator();
while(iterator.hasNext()) {
LifecycleRule rule = iterator.next();
if (rule.getAction().getActionType().equals(LifecycleRule.DeleteLifecycleAction.TYPE)) {
iterator.remove();
}
}
remoteBucket.toBuilder().setLifecycleRules(lifecycleRules).build().update();
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.
Did this help? I think we can merge this change once you add a helper to Storage interface but no change is required for StorageRpc.java.
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.
@frankyn thanks for this, I tried your sample code but its not working please check the below response.
[
LifecycleRule{
lifecycleAction=DeleteLifecycleAction{
actionType=Delete
},
lifecycleCondition=LifecycleCondition{
age=1,
createBefore=null,
numberofNewerVersions=null,
isLive=null,
matchesStorageClass=null
}
},
LifecycleRule{
lifecycleAction=SetStorageClassLifecycleAction{
actionType=SetStorageClass,
storageClass=COLDLINE
},
lifecycleCondition=LifecycleCondition{
age=1,
createBefore=2020-05-15,
numberofNewerVersions=3,
isLive=false,
matchesStorageClass=[
COLDLINE
]
}
}
]
does it works on your end? i think bucket lifecycle rule's not updated properly.
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.
storage.delete(bucketName)
is clean up for the example and not required by remove lifecycle rule.
Please let me know if there is still confusion with the workaround. In short, the library should not require the workaround.
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.
@frankyn thanks for the clarification. It's works i will raise separate PR by adding these helper to Storage interface.
Thank you for your help.
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.
Thank you! I appreciate your patience.
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.
@athakor I might be confused, you're going to close this PR and make another right?
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.
yes, will close this once newly created PR gets Approved
…erules from bucket
…ete lifecycle rules
Fixes #20