Storage: add lifecycle rules helpers to bucket

[tseaver]

@frankyn Note that the system test added in the last commit currently fails. It is sending the POST body as

{'lifecycle': {'rule': [{'action': {'type': 'Delete'},
                         'condition': {'age': 42}},
                        {'action': {'type': 'SetStorageClass'},
                         'condition': {'isLive': False,
                                       'matchesStorageClass': 'NEARLINE'},
                         'storageClass': 'COLDLINE'}]},
 'name': 'w-lifcycle-rules-1535755546278'}

and fails with a 400:

{'error': {'code': 400,
           'errors': [{'domain': 'global',
                       'message': 'Invalid argument',
                       'reason': 'invalid'}],
           'message': 'Invalid argument'}}

I'd like help tracking down what is causing that error from the back-end's perspective.

[frankyn]

@tseaver take a look at the write up again. I added additional information on tests. This is a case that should fail for setstorageclass if storage class is missing from action. One follow up for me is a better error message.

[tseaver]

@frankyn Please confirm that storageClass is supposed to be part of action: the API docs note it as an element of action, while the "GCS Object Lifecycle Management" one pager notes storageClass as peer of action and condition.

[tseaver]

When I move setStorageClass inside action, the POSTed resource looks like:

{'lifecycle': {'rule': [{'action': {'type': 'Delete'},
                         'condition': {'age': 42}},
                        {'action': {'storageClass': 'COLDLINE',
                                    'type': 'SetStorageClass'},
                         'condition': {'isLive': False,
                                       'matchesStorageClass': 'NEARLINE'}}]},
 'name': 'w-lifcycle-rules-1535767555303'}

and the 400 error response like this:

{'error': {'code': 400,
           'errors': [{'domain': 'global',
                       'message': 'Invalid location constraint ""',
                       'reason': 'invalid'}],
           'message': 'Invalid location constraint ""'}}

[frankyn]

I fixed the mistake in the document. Thanks for catching it.

Expected request:

{'lifecycle': {'rule': [{'action': {'type': 'Delete'},
                         'condition': {'age': 42}},
                        {'action': {'storageClass': 'COLDLINE',
                                    'type': 'SetStorageClass'},
                         'condition': {'isLive': False,
                                       'matchesStorageClass': ['NEARLINE']}}]},
 'name': 'w-lifcycle-rules-1535767555303'}

The difference is matchesStorageClass expects an array of storage classes.

[tseaver]

@frankyn I've moved 'setStorageClassinsideaction, and updated matches_storage_class` to document that it is a list, and pass lists everywhere. However, the system test still fails with:

(Pdb) pp properties
{'lifecycle': {'rule': [{'action': {'type': 'Delete'},
                         'condition': {'age': 42}},
                        {'action': {'storageClass': 'COLDLINE',
                                    'type': 'SetStorageClass'},
                         'condition': {'isLive': False,
                                       'matchesStorageClass': ['NEARLINE']}}]},
 'name': 'w-lifcycle-rules-1536091244549'}

(pdb) pp response.json()
{'error': {'code': 400,
           'errors': [{'domain': 'global',
                       'message': 'Invalid location constraint ""',
                       'reason': 'invalid'}],
           'message': 'Invalid location constraint ""'}}

[frankyn]

@tseaver I was able to set the policy given (but needed to change False -> false).
Is the policy you provided above the raw request body?

[tseaver]

@frankyn

Is the policy you provided above the raw request body?

The values I showed are the Python dict before serializing to JSON. The dumped JSON looks like:

(Pdb) pp data
('{"lifecycle": {"rule": [{"action": {"type": "Delete"}, "condition": {"age": '
 '42}}, {"action": {"type": "SetStorageClass", "storageClass": "COLDLINE"}, '
 '"condition": {"isLive": false, "matchesStorageClass": ["NEARLINE"]}}]}, '
 '"name": "w-lifcycle-rules-1536096086990"}')

[frankyn]

face palm, I was able to repro. This seems like a bug, but not yet sure. The part I missed was including the lifecycle policy on a bucket create operation.

[frankyn]

By including location for the bucket "location": "us" it was able to create using the provided lifecycle policy. Will follow-up internally, it might be WAI, but if lifecycle isn't included location isn't required (also documented in ref guide: "Defaults to US".

[tseaver]

@frankyn OK, fingers crossed that a534067 gets the system test passing.

[frankyn]

@tseaver, looking at the surface,
What are your thoughts on a surface similar to:

from google.cloud import storage

storage_client = storage.Client()

bucket = storage_client.bucket("bucket-name")

bucket.add_lifecycle_delete_rule(...conditions..)

# OR
# this method may be too long
bucket.add_lifecycle_set_storage_class(storage_class, ...conditions...)

In implementation, the methods can construct LifecycleRuleDelete or LifecycleRuleSetStorageClass and update bucket.lifecycle_rules for the bucket instance.

I'm open to thoughts. Apologies for not providing this feedback sooner.

[tseaver]

@frankyn 1e04aa6 96adbf6 adds the two bucket helper methods you suggested (add_lifecycle_rule_delete and add_lifecycle_rule_set_storage_class), along with clear_lifecycle_rules (which seems best for parity / clarity).

[tseaver]

@frankyn I'd argue that adding a systest which tries to provoke a 400 is pointless for Python: we expect the underlying layers to convert / raise them automagically. Also, the back-end bug where creating a bucket w/ lifecycle rules but w/o an explicit location could be fixed, which would then break the systest.

[frankyn]

(w.r.t to tests) The reason to test failure was to test behavior of client, but I'm open to removing this expectation. It seems unnecessary at this point.

(w.r.t to the surface) The surface LGTM just have a question.

I'm curious about the lifecycle of bucket.lifecycle_rules() with the update:
Are the following two ways a user can now interact with rules (keeping backward compat)?

# new
bucket = storage_client.get_bucket("bucket-name")
for rule in bucket.lifecycle_rules:
  print(rule.action)
  print(rule.conditions.age)

OR

#existing
bucket = storage_client.get_bucket("bucket-name")
for rule in bucket.lifecycle_rules:
  print(rule["action"]
  print(rule["conditions"]["age"])

[tseaver]

@frankyn Both LifecycleRuleDelete and LifecycleRuleSetStorageClass are derived from dict, so the "existing" example works (well, the key in the dict is condition, not conditions, but that is what the API expects).

Neither one defines an action or a conditions property / attribute.

[frankyn]

Thanks @tseaver, additional comments on documentation.

[tseaver]

Merging after an "LGTM" from @frankyn in chat.

[frankyn]

Thanks Tres!