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

Setting lifecycle rules totally undocumented #2610

Closed
mlissner opened this issue Oct 25, 2016 · 10 comments
Closed

Setting lifecycle rules totally undocumented #2610

mlissner opened this issue Oct 25, 2016 · 10 comments
Assignees
Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@mlissner
Copy link
Contributor

It took me quite a while to figure out how to set lifecycle rules. If you look at the code, it's undocumented:

@lifecycle_rules.setter
def lifecycle_rules(self, rules):
    self._patch_property('lifecycle', {'rule': rules})

I found some tests for this, but even those don't seem to use the lifecycle_rules method. Not having a documented method for doing this is very frustrating since this is supposed to be a library that supports all of the features.

@mlissner mlissner changed the title Lifecycle rules totally undocumented Setting lifecycle rules totally undocumented Oct 25, 2016
@daspecster daspecster added docs api: storage Issues related to the Cloud Storage API. labels Oct 25, 2016
@daspecster
Copy link
Contributor

@mlissner, thanks for pointing this out.

This library is still under heavy development and hasn't reached 1.0 yet, which is why some areas are missing or light on documentation. There are also a number of features that aren't yet supported by this library.

@dhermes
Copy link
Contributor

dhermes commented Oct 25, 2016

@mlissner That was the setter, you want the getter. Is your concern that the documentation for the property is insufficient / doesn't indicate that there is a setter?

@tseaver
Copy link
Contributor

tseaver commented Oct 25, 2016

In fact, the @property descriptor explicitly takes the __doc__ from the getter: any docstring on from the setter or deleter are not discoverable at runtime.

@mlissner
Copy link
Contributor Author

Is your concern that the documentation for the property is insufficient / doesn't indicate that there is a setter?

Yes, exactly. I want to set the lifecycle rules for a bucket, but there's no documentation of how to do so.

@mlissner
Copy link
Contributor Author

OK, I don't know where else to put this, so I'm putting it here. I've struggled mightily to figure out this API and I hope this feedback will be helpful. Two things:

  1. To set the lifecycle_rules, you need to set them before you create the bucket:

    b = client.bucket('my-bucket-name')
    b.exists()  # False
    b.lifecycle_rules = [{
        'action': {'type': 'Delete'},
        'condition': {'age': 1},
    }]
    b.create()
    

    It doesn't seem to be possible to set lifecycle rules for an existing bucket, though I'd love to be proven wrong. If this is possible, it's certainly not documented anywhere I could find. I'd expect that a setter would actually set the values on in Google Cloud or that there would be a commit command somewhere to commit the changes (I haven't found this...yet). Since neither is the case, maybe setting these values on an existing bucket should throw an exception?

  2. After you've created a bucket like this, you can test that the creation worked by using the reload method on a bucket:

    b = client.bucket('my-bucket-name')
    b.lifecycle_rules  # Returns empty list: [], WTF?
    b.reload()
    b.lifecycle_rules  # Returns the rules from above. Success!
    

    Since reload is also undocumented, this is extremely difficult to figure out, since for all intents and purposes, it looks like your lifecycle rules didn't get created when you check them. The only thing that tipped me off was that there didn't seem to be any network latency when I was trying to get the rules. If I had been on a faster connection, this would have been even harder to detect.

    Do you want another ticket for documenting the reload function? I'm new to Google Cloud, but this whole thing is extremely frustrating. There are some big gaps here in documentation and user expectation.

@pdknsk
Copy link

pdknsk commented Oct 26, 2016

In fact, the @property descriptor explicitly takes the __doc__ from the getter: any docstring on from the setter or deleter are not discoverable at runtime.

This is basically a duplicate of #1838.

that there would be a commit command somewhere to commit the changes

It's named patch.

@tseaver
Copy link
Contributor

tseaver commented Oct 26, 2016

@mlissner Thanks for digging deeper! Addressing a couple of your points:

  • Changes made to the client-side Bucket object do not implicitly trigger API calls to propagate those changes to the back-end. This is a typical "distributed system" tradeoff. Accumulated changes can be pushed to the back-end via bucket.patch().
  • As you noted, Bucket.reload does not appear in the Bucket API docs: the source of the problem is that Bucket.reload (and Bucket.patch) are actually inherited from a base class shared with Blob, and the Sphinx configuration doesn't specify including inherited members. I have just opened Surface inherited members for Bucket/Blob. #2621 to correct this issue.
  • Another related issue is that we don't have narrative usage docs for Storage: they might have made the expected pattern clearer, even in the absence of the inherited members.

@mlissner
Copy link
Contributor Author

Thanks @tseaver and @pdknsk. So I'm seeing three separate issues:

  1. Setters undocumented (to be addressed in CORS documentation missing setter #1838)
  2. Inherited members like reload and patch are not documented (addressed in Surface inherited members for Bucket/Blob. #2621, yay!).
  3. No narrative usage docs for Storage. This is still not addressed, and I agree this would make a huge difference. Should we close this and create a new issue for that?

@mlissner
Copy link
Contributor Author

Following up: setters are now documented for CORS, per #1838, but we still need another PR to fix lifecycle_rules, and we still need narrative docs for Storage.

@lukesneeringer lukesneeringer added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 19, 2017
richkadel pushed a commit to richkadel/google-cloud-python that referenced this issue May 6, 2017
@tseaver
Copy link
Contributor

tseaver commented Jun 5, 2017

PR #1420 (long stalled) makes a stab at adding usage docs for storage. #3472 adds the setter documentation to lifecycle_rules and versioning_enabled.

tseaver added a commit that referenced this issue Jun 7, 2017
* Expose that settable properties are so.

Closes #2610.
landrito pushed a commit to landrito/google-cloud-python that referenced this issue Aug 21, 2017
* Expose that settable properties are so.

Closes googleapis#2610.
landrito pushed a commit to landrito/google-cloud-python that referenced this issue Aug 22, 2017
* Expose that settable properties are so.

Closes googleapis#2610.
landrito pushed a commit to landrito/google-cloud-python that referenced this issue Aug 22, 2017
* Expose that settable properties are so.

Closes googleapis#2610.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

6 participants