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
fix: replace python lifecycle action parsing ValueError with warning #437
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds much better. Thanks for the review! |
||
rule | ||
), | ||
UserWarning, | ||
stacklevel=1, | ||
) | ||
|
||
@lifecycle_rules.setter | ||
def lifecycle_rules(self, rules): | ||
|
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.
Does it cause issues if nothing is yielded here?
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.
Let me double check on that, thanks! Any insight on this @andrewsg?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the point of future proof, I think suggesting upgrading to a newer client version would make sense. Will incorporate that in!