-
Notifications
You must be signed in to change notification settings - Fork 387
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
Conversation
2165453
to
963098c
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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.
Reviewed 5 of 5 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tritone)
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.
Looks good!
- Would we want to add Archive to the integration test as well? See
// Create a request to update the metadata, change the storage class because - 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).
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.
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. |
|
This fixes #3075
This change is