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

Jump to bottom

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

Merged
merged 4 commits into from May 8, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a20476e
fix: replace python lifecycle action parsing ValueError with warning
cojenco May 7, 2021
013c9c6
fix lint
cojenco May 7, 2021
6ef73da
add client upgrade suggestion to unknown OLM rule warning
cojenco May 7, 2021
a66e45d
update warning message
cojenco May 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion google/cloud/storage/bucket.py
View file
Open in desktop
Expand Up @@ -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(
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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!

"Unknown lifecycle rule type received: {}. Please upgrade to the latest version of google-cloud-storage.".format(
rule
),
UserWarning,
stacklevel=1,
)

@lifecycle_rules.setter
def lifecycle_rules(self, rules):
Expand Down
13 changes: 10 additions & 3 deletions tests/unit/test_bucket.py
View file
Open in desktop
Expand Up @@ -1783,15 +1783,22 @@ def test_iam_configuration_policy_w_entry(self):
self.assertTrue(config.uniform_bucket_level_access_enabled)
self.assertEqual(config.uniform_bucket_level_access_locked_time, now)

def test_lifecycle_rules_getter_unknown_action_type(self):
@mock.patch("warnings.warn")
def test_lifecycle_rules_getter_unknown_action_type(self, mock_warn):
NAME = "name"
BOGUS_RULE = {"action": {"type": "Bogus"}, "condition": {"age": 42}}
rules = [BOGUS_RULE]
properties = {"lifecycle": {"rule": rules}}
bucket = self._make_one(name=NAME, properties=properties)

with self.assertRaises(ValueError):
list(bucket.lifecycle_rules)
list(bucket.lifecycle_rules)
mock_warn.assert_called_with(
"Unknown lifecycle rule type received: {}. Please upgrade to the latest version of google-cloud-storage.".format(
BOGUS_RULE
),
UserWarning,
stacklevel=1,
)

def test_lifecycle_rules_getter(self):
from google.cloud.storage.bucket import (
Expand Down