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 ARCHIVE storage class #2428

Merged
merged 3 commits into from
Dec 30, 2019
Merged

feat: add support for ARCHIVE storage class #2428

merged 3 commits into from
Dec 30, 2019

Conversation

jdpedrie
Copy link
Contributor

Closes #2425

cc @tritone for review.

@jdpedrie jdpedrie added the api: storage Issues related to the Cloud Storage API. label Oct 28, 2019
@jdpedrie jdpedrie requested a review from dwsupplee as a code owner October 28, 2019 13:56
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2019
@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #2428 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2428      +/-   ##
============================================
+ Coverage     92.47%   92.55%   +0.07%     
- Complexity     4477     4504      +27     
============================================
  Files           312      312              
  Lines         13416    13496      +80     
============================================
+ Hits          12407    12491      +84     
+ Misses         1009     1005       -4
Impacted Files Coverage Δ Complexity Δ
Storage/src/StorageClient.php 100% <ø> (ø) 31 <0> (ø) ⬇️
Storage/src/Lifecycle.php 70% <ø> (ø) 20 <0> (ø) ⬇️
Storage/src/Bucket.php 97.18% <ø> (ø) 70 <0> (ø) ⬇️
Firestore/src/Query.php 99.2% <0%> (-0.01%) 86% <0%> (ø)
Translate/src/Connection/ConnectionInterface.php
Translate/src/TranslateClient.php 100% <0%> (ø) 23% <0%> (?)
Translate/src/Connection/Rest.php 100% <0%> (+100%) 4% <0%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca739c4...0b03b09. Read the comment docs.

Copy link

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Test looks good, had a comment about the documentation piece. Also, please wait to merge this as the feature is still behind a flag (I will give a heads up when it's ready in a couple weeks).

@@ -90,7 +90,8 @@ public function __construct(array $lifecycle = [])
* @type string[] $matchesStorageClass Objects having any of the storage
* classes specified by this condition will be matched. Values
* include `"MULTI_REGIONAL"`, `"REGIONAL"`, `"NEARLINE"`,
Copy link

Choose a reason for hiding this comment

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

It would be good to re-sort this list as follows: STANDARD, NEARLINE, COLDLINE, ARCHIVE. Then note that MULTI_REGIONAL, REGIONAL, DURABLE_REDUCED_AVAILABILITY are legacy (ie not recommended for new implementations). Should look similar to the comment here in the ruby client library, plus the addition of ARCHIVE: https://github.com/googleapis/google-cloud-ruby/blob/master/google-cloud-storage/lib/google/cloud/storage/bucket.rb#L358 . You can also add a link to the docs about storage classes here (if you don't think it's too much info for this context): https://cloud.google.com/storage/docs/storage-classes

Same thing for StorageClient.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@jdpedrie jdpedrie added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 28, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2019
@tritone
Copy link

tritone commented Oct 28, 2019

Looks good to me; will give approval once the feature is no longer behind a flag. Also needs approval from a repo owner. Thanks!

@tritone
Copy link

tritone commented Dec 27, 2019

We are clear to merge this now with a plan to release the change between 1/2 and 1/8. It looks like there are two remaining things now to be resolved:

  • @dwsupplee , we need repo owner approval from you on this PR.
  • Looks like the code coverage check is also currently blocking the merge. @jdpedrie could you look into that?

Let me know if either of you see any issue with this timeline.

@jdpedrie
Copy link
Contributor Author

@tritone just rebased this, should fix the coverage check.

Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

Looks good! Just some small notes.

@jdpedrie
Copy link
Contributor Author

Thanks @dwsupplee. Went back to including the double-quotes, since I think that's more in line with the general formatting we've preferred til now.

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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage: Add support for archive storage class
5 participants