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

fix: replace python lifecycle action parsing ValueError with warning #437

Merged
merged 4 commits into from May 8, 2021

Conversation

@cojenco
Copy link
Contributor

@cojenco cojenco commented May 7, 2021

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.

@google-cla google-cla bot added the cla: yes label May 7, 2021
@cojenco cojenco marked this pull request as ready for review May 7, 2021
@cojenco cojenco requested a review from May 7, 2021
@cojenco cojenco requested a review from as a code owner May 7, 2021
Copy link
Contributor

@andrewsg andrewsg left a comment

Thanks!

@@ -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(
Copy link
Contributor

@tritone tritone May 7, 2021

Does it cause issues if nothing is yielded here?

Copy link
Contributor Author

@cojenco cojenco May 7, 2021

Let me double check on that, thanks! Any insight on this @andrewsg?

Copy link
Contributor

@andrewsg andrewsg May 7, 2021

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.

Copy link
Contributor

@tritone tritone May 7, 2021

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?

Copy link
Contributor Author

@cojenco cojenco May 7, 2021

To the point of future proof, I think suggesting upgrading to a newer client version would make sense. Will incorporate that in!

tritone
tritone approved these changes May 8, 2021
@@ -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(
Copy link
Contributor

@tritone tritone May 8, 2021

I would say something like Unknown lifecycle rule type received: {}. Please upgrade to the latest version of google-cloud-storage.

How does that sound?

Copy link
Contributor Author

@cojenco cojenco May 8, 2021

That sounds much better. Thanks for the review!

@cojenco cojenco merged commit 2532d50 into master May 8, 2021
6 checks passed
@cojenco cojenco deleted the lifecycle-009 branch May 8, 2021
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…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
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants