Skip to content
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

Closed
wants to merge 21 commits into from

Conversation

athakor
Copy link
Contributor

@athakor athakor commented Jan 3, 2020

Fixes #20

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 3, 2020
@codecov

This comment has been minimized.

@JesseLovelace
Copy link
Contributor

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

@athakor
Copy link
Contributor Author

athakor commented Jan 6, 2020

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 disableLifecycleRules.

@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 6, 2020
Copy link
Contributor

@JesseLovelace JesseLovelace left a 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

@athakor
Copy link
Contributor Author

athakor commented Jan 9, 2020

@JesseLovelace PTAL

@JesseLovelace
Copy link
Contributor

Also, your IT is failing because you aren't using a real service account email

@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 17, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 17, 2020
@JesseLovelace
Copy link
Contributor

Try using storage.getServiceAccount(projectId) instead of extracting it from the json like you're doing now

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2020
@athakor
Copy link
Contributor Author

athakor commented Jan 23, 2020

Try using storage.getServiceAccount(projectId) instead of extracting it from the json like you're doing now

done @JesseLovelace PTAL

@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 31, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 31, 2020
@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 3, 2020
@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 7, 2020
@athakor
Copy link
Contributor Author

athakor commented Feb 7, 2020

@JesseLovelace PTAL

@athakor
Copy link
Contributor Author

athakor commented Feb 14, 2020

@frankyn @JesseLovelace gentle ping.

@frankyn
Copy link
Member

frankyn commented Feb 14, 2020

@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

@JesseLovelace JesseLovelace added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 26, 2020
@athakor
Copy link
Contributor Author

athakor commented Mar 31, 2020

@frankyn @JesseLovelace here, how we can move ahead with above situation?

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussing this now in #179, looks like we've been making interface breaking changers and not bumping java major version. Apologies for the delay @athakor

google-cloud-storage/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@@ -3208,4 +3209,36 @@ public void testBucketLogging() throws ExecutionException, InterruptedException
RemoteStorageHelper.forceDelete(storage, loggingBucket, 5, TimeUnit.SECONDS);
}
}

@Test
public void testDeleteLifecycleRules() throws ExecutionException, InterruptedException {
Copy link
Member

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?

Copy link
Contributor Author

@athakor athakor Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankyn

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 or disableLifecycleRules instead of deleteLifecycleRules 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.

Copy link
Member

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();

Copy link
Member

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.

Copy link
Contributor Author

@athakor athakor May 15, 2020

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

@frankyn frankyn Jun 2, 2020

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?

Copy link
Contributor Author

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

@frankyn frankyn requested a review from jkwlui May 14, 2020 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud Storage: Removing lifecyclerules from bucket is not working
6 participants