fix: replace python lifecycle action parsing ValueError with warning #437
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
| @@ -2361,7 +2361,9 @@ def lifecycle_rules(self): | |||
| elif action_type == "SetStorageClass": | |||
| yield LifecycleRuleSetStorageClass.from_api_repr(rule) | |||
| else: | |||
| raise ValueError("Unknown lifecycle rule: {}".format(rule)) | |||
| warnings.warn( | |||
There was a problem hiding this comment.
Does it cause issues if nothing is yielded here?
There was a problem hiding this comment.
Let me double check on that, thanks! Any insight on this @andrewsg?
There was a problem hiding this comment.
I considered that in my review but decided it looked to me like because it's in a for loop, yielding nothing will behave identically to no rule at all, which is probably acceptable in this case. I'm not familiar with how people use this feature, however.
There was a problem hiding this comment.
Gotcha, I guess it will just continue to the next rule in the list and/or end the generator if nothing is left.
I was discussing this issue with someone who pointed me to this code in the Java Bigtable client: https://github.com/googleapis/java-bigtable/blob/master/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/EncryptionInfo.java#L48 . I like the message there-- could we also have the warning suggest upgrading to a newer client version?
There was a problem hiding this comment.
To the point of future proof, I think suggesting upgrading to a newer client version would make sense. Will incorporate that in!
cojenco
added
the
do not merge
google/cloud/storage/bucket.py
Outdated
| @@ -2361,7 +2361,13 @@ def lifecycle_rules(self): | |||
| elif action_type == "SetStorageClass": | |||
| yield LifecycleRuleSetStorageClass.from_api_repr(rule) | |||
| else: | |||
| raise ValueError("Unknown lifecycle rule: {}".format(rule)) | |||
| warnings.warn( | |||
| "Unknown lifecycle rule by the client: {}. Please upgrade your client.".format( | |||
There was a problem hiding this comment.
I would say something like Unknown lifecycle rule type received: {}. Please upgrade to the latest version of google-cloud-storage.
How does that sound?
There was a problem hiding this comment.
That sounds much better. Thanks for the review!
cojenco
added
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
…oogleapis#437) * fix: replace python lifecycle action parsing ValueError with warning * fix lint * add client upgrade suggestion to unknown OLM rule warning * update warning message
…oogleapis#437) * fix: replace python lifecycle action parsing ValueError with warning * fix lint * add client upgrade suggestion to unknown OLM rule warning * update warning message
Replace python Lifecycle action parsing ValueError with a user warning in cases where the client attempts to parse unexpected fields. This will help future proof this feature by preventing runtime errors for parsing unsupported actions.