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

feat: add support for 'Blob.custom_time' and lifecycle rules #199

Merged
merged 22 commits into from Aug 26, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #196

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 1, 2020
@HemangChothani HemangChothani changed the title Storage issue 196 feat(storage): add support of custom time metadata and timestamp Jul 1, 2020
Copy link
Member

@jkwlui jkwlui left a 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.

@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 24, 2020
@tseaver
Copy link
Contributor

tseaver commented Jul 24, 2020

I have marked this PR as do not merge per @jkwlui .

google/cloud/storage/blob.py Outdated Show resolved Hide resolved
tests/unit/test_blob.py Show resolved Hide resolved
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
Copy link
Contributor

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).

@HemangChothani HemangChothani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2020
@busunkim96 busunkim96 closed this Jul 31, 2020
@busunkim96 busunkim96 reopened this Jul 31, 2020
@tseaver tseaver changed the title feat(storage): add support of custom time metadata and timestamp feat: add support for 'Blob.custom_time' and lifecyccle rules. Aug 4, 2020
@tseaver tseaver changed the title feat: add support for 'Blob.custom_time' and lifecyccle rules. feat: add support for 'Blob.custom_time' and lifecyccle rules Aug 4, 2020
Copy link
Member

@frankyn frankyn left a 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

@HemangChothani
Copy link
Contributor Author

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.

@frankyn That change has been already done.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be dateTime

Copy link
Member

@frankyn frankyn left a 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.


See https://cloud.google.com/storage/docs/json_api/v1/objects

:type value: :class:`datetime.datetime` or ``NoneType``
Copy link
Member

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.

Copy link
Member

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.


: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.
Copy link
Member

@frankyn frankyn Aug 24, 2020

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.

@frankyn frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 24, 2020
Copy link
Member

@frankyn frankyn left a 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


See https://cloud.google.com/storage/docs/json_api/v1/objects

:type value: :class:`datetime.datetime` or ``NoneType``
Copy link
Member

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.

@frankyn frankyn requested a review from tseaver August 26, 2020 17:19
@frankyn frankyn changed the title feat: add support for 'Blob.custom_time' and lifecyccle rules feat: add support for 'Blob.custom_time' and lifecycle rules Aug 26, 2020
@frankyn frankyn dismissed tseaver’s stale review August 26, 2020 18:00

tseaver's feedback was addressed and dismissing as the PR LGTM.

@frankyn frankyn merged commit 180873d into googleapis:master Aug 26, 2020

@custom_time.setter
def custom_time(self, value):
"""Set the custom time for the object. Once set it can't be unset
Copy link
Contributor

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.
Copy link
Contributor

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")
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setter for this property?

Copy link
Contributor Author

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.

Copy link
Member

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.

cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…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>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support customTime metadata and custom timestamp based OLM
7 participants