-
Notifications
You must be signed in to change notification settings - Fork 154
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
feat: add support for 'Blob.custom_time' and lifecycle rules #199
Conversation
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.
Thanks @HemangChothani! Please hold until product is ready to move forward.
I have marked this PR as |
google/cloud/storage/blob.py
Outdated
See https://cloud.google.com/storage/docs/json_api/v1/objects | ||
|
||
:type value: :class:`datetime.datetime` | ||
:param value: (Optional) Set the custom time of blob. Datetime object parsed |
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.
Document the semantics of None
value (which should be to clear any existing property value).
…-storage into storage_issue_196
…into storage_issue_196 Y
1d96369
to
0bd9bf4
Compare
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.
Thanks for your patience @HemangChothani,
Update: The Cloud Storage team has updated type of noncurrent_time_before and custom_time to be a Date format (YYYY-mm-dd) instead of DateTime.
Discovery document with updated documentation with respect to these fields:
https://www.googleapis.com/discovery/v1/apis/storage/v1/rest
@frankyn That change has been already done. |
…into storage_issue_196
google/cloud/storage/blob.py
Outdated
See https://cloud.google.com/storage/docs/json_api/v1/objects | ||
|
||
:rtype: :class:`datetime.date` or ``NoneType`` | ||
:returns: Date object parsed from iso8601 valid date, or |
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.
This should be dateTime
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.
Have two nits for documentation, otherwise LGTM.
google/cloud/storage/blob.py
Outdated
|
||
See https://cloud.google.com/storage/docs/json_api/v1/objects | ||
|
||
:type value: :class:`datetime.datetime` or ``NoneType`` |
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.
Once set it can't be unset and only changed to a custom datetime in the future. If the custom_time must be unset, you must either perform a rewrite operation or upload the data again.
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'd state that setter can only be datetime.datetime to not confuse folks.
google/cloud/storage/bucket.py
Outdated
|
||
:type custom_time_before: :class:`datetime.date` | ||
:param custom_time_before: (Optional) Apply rule action to items whose custom time is before this | ||
date. This condition is relevant only for versioned objects. |
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.
Please include similar text to noncurrent_time_before with RFC3339 and an example.
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.
1 nit, thanks @HemangChothani
google/cloud/storage/blob.py
Outdated
|
||
See https://cloud.google.com/storage/docs/json_api/v1/objects | ||
|
||
:type value: :class:`datetime.datetime` or ``NoneType`` |
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'd state that setter can only be datetime.datetime to not confuse folks.
tseaver's feedback was addressed and dismissing as the PR LGTM.
|
||
@custom_time.setter | ||
def custom_time(self, value): | ||
"""Set the custom time for the object. Once set it can't be unset |
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.
Please follow PEP 257 for Multi-line docstrings, i.e., move the non-summary down below.
"""Set the custom time for the object. Once set it can't be unset | ||
and only changed to a custom datetime in the future. If the | ||
custom_time must be unset, you must either perform a rewrite operation | ||
or upload the data again. |
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.
These semantics are not enforced by the method.
@property | ||
def days_since_custom_time(self): | ||
"""Conditon's 'days_since_custom_time' value.""" | ||
return self.get("daysSinceCustomTime") |
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.
Setter for this property?
"""Conditon's 'custom_time_before' value.""" | ||
before = self.get("customTimeBefore") | ||
if before is not None: | ||
return datetime_helpers.from_iso8601_date(before) |
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.
Setter for this property?
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.
Not sure about the setter method for all this properties, because we don't have a method for other properties as well which defined before. Please let me know if needed, so need to create setter method for all the properties of LifecycleRuleConditions
class.
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.
Thanks both, looks like setters weren't defined at this level. It will be filed as a feature request for now. It doesn't block customers from using the feature.
…pis#199) * feat(storage): add support of custom time metadata and timestamp * feat(storage): change the return type of custom_time_before * feat(storage): add setter method * feat(storage): add test for None value * feat(storage): changes in unittest * feat(storage): change custom_time type to date * feat: change custom_time to datetime * feat: nit Co-authored-by: Jonathan Lui <jonathanlui@google.com> Co-authored-by: Tres Seaver <tseaver@palladion.com> Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
…pis#199) * feat(storage): add support of custom time metadata and timestamp * feat(storage): change the return type of custom_time_before * feat(storage): add setter method * feat(storage): add test for None value * feat(storage): changes in unittest * feat(storage): change custom_time type to date * feat: change custom_time to datetime * feat: nit Co-authored-by: Jonathan Lui <jonathanlui@google.com> Co-authored-by: Tres Seaver <tseaver@palladion.com> Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Fixes #196