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:new helper function for 'archive' storage class #3197

Merged
merged 1 commit into from
Oct 14, 2019

Conversation

manish-qlogic
Copy link
Contributor

@manish-qlogic manish-qlogic commented Oct 13, 2019

This fixes #3075


This change is Reviewable

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2019
@codecov
Copy link

codecov bot commented Oct 13, 2019

Codecov Report

Merging #3197 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3197      +/-   ##
==========================================
- Coverage   91.06%   91.02%   -0.04%     
==========================================
  Files         300      300              
  Lines       20360    20379      +19     
==========================================
+ Hits        18540    18551      +11     
- Misses       1820     1828       +8
Impacted Files Coverage Δ
google/cloud/storage/storage_class.h 100% <100%> (ø) ⬆️
google/cloud/storage/lifecycle_rule.cc 100% <100%> (ø) ⬆️
google/cloud/storage/lifecycle_rule.h 100% <100%> (ø) ⬆️
google/cloud/storage/internal/complex_option.h 66.66% <0%> (-16.67%) ⬇️
google/cloud/storage/well_known_headers.h 85.18% <0%> (-7.41%) ⬇️
google/cloud/storage/well_known_parameters.h 70% <0%> (-6.67%) ⬇️
...le/cloud/bigtable/internal/async_retry_unary_rpc.h 86.79% <0%> (-1.67%) ⬇️
google/cloud/bigtable/row_range.h 100% <0%> (ø) ⬆️
google/cloud/bigtable/app_profile_config.h 100% <0%> (ø) ⬆️
google/cloud/storage/internal/common_metadata.h 97.82% <0%> (+0.76%) ⬆️
... and 2 more

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 8ab5e51...963098c. Read the comment docs.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tritone)

@coryan coryan merged commit 9928674 into googleapis:master Oct 14, 2019
Copy link
Contributor

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

Looks good!

@coryan
Copy link
Contributor

coryan commented Oct 14, 2019

Maybe, the goal of that integration test is to verify we can change the storage class field value, any valid value would be acceptable in the test. Feel free to open a separate issue if you think it is important.

  • In the future we should probably wait to merge until the feature is no longer behind a flag (though not a big deal this time).

I disagree. In general, we merge as soon as possible, and will continue to do so. Gating on everything to be "just so" is a sure way to kill velocity.

@tritone
Copy link
Contributor

tritone commented Oct 14, 2019

  1. Makes sense, no need to add it to the integration test in that case.
  2. I definitely understand that you don't want to leave PRs sitting unresolved, but it does seem like it would be disorienting to users that we allow them to make calls which will fail because the feature is not ready yet. Would it be preferable to just assign the work later in a situation like this? Or do you think it's not a big deal from a user perspective? I guess it's only a matter of weeks until release...

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.

Create new helper function for "archive" storage class.
4 participants