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(storage): support soft delete timestamps #13728

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Mar 4, 2024

Part of the work for #13575


This change is Reviewable

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Mar 4, 2024
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.27%. Comparing base (95fce04) to head (20e71ac).

❗ Current head 20e71ac differs from pull request most recent head ab544ef. Consider uploading reports for the commit ab544ef to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13728   +/-   ##
=======================================
  Coverage   93.26%   93.27%           
=======================================
  Files        2229     2229           
  Lines      193104   193184   +80     
=======================================
+ Hits       180105   180184   +79     
- Misses      12999    13000    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coryan coryan marked this pull request as ready for review March 4, 2024 23:56
@coryan coryan requested review from a team as code owners March 4, 2024 23:56
return *this;
}

/// Returns true if the object has a soft delete timestamp.
Copy link
Member

Choose a reason for hiding this comment

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

s/soft/hard/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

google/cloud/storage/object_metadata.h Outdated Show resolved Hide resolved
return *this;
}

/// @note THis is only intended for mocking.
Copy link
Member

Choose a reason for hiding this comment

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

s/THis/This/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -505,6 +505,50 @@ class ObjectMetadata {
return *this;
}

/// Returns true if the object has a soft delete timestamp.
bool has_soft_delete_time() const { return soft_delete_time_.has_value(); }
Copy link
Member

Choose a reason for hiding this comment

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

Why not have one method that returns the optional<>?

Is it consistency with has_owner(), for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All other timestamps (and all other scalars really) return a default value when not set. If we had a time machine I would probably go back and do something different, but I am not sure it is worth changing the approach on only new fields.

@coryan coryan enabled auto-merge (squash) March 5, 2024 19:24
@coryan coryan merged commit ffaefee into googleapis:main Mar 5, 2024
62 checks passed
@coryan coryan deleted the feat-storage-support-soft-delete-metadata-fields branch March 5, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants