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.

Sign up for GitHub

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

Jump to bottom

fix(storage): allow to use age=0 in OLM conditions #6204

Merged
merged 10 commits into from Aug 26, 2022
Merged

fix(storage): allow to use age=0 in OLM conditions #6204

merged 10 commits into from Aug 26, 2022

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Jun 16, 2022 •
edited

Storage Go doesn't allow setting AgeInDays=0 after discussing with @tritone. This PR is the agreed upon path to support Age=0 in OLM conditions. It also notes that other int fields in OLM Conditions do not support 0 values.

Fixes: #6539, #6240

@frankyn frankyn requested review from a team as code owners June 16, 2022 20:38
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jun 16, 2022
@codyoss codyoss changed the title fix: allow to use age=0 in OLM conditions fix(storage): allow to use age=0 in OLM conditions Jun 16, 2022
tritone
tritone reviewed Jun 16, 2022
View reviewed changes
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.

Thanks Frank!

storage/bucket.go Outdated Show resolved Hide resolved
storage/bucket.go Outdated Show resolved Hide resolved
storage/bucket.go Outdated Show resolved Hide resolved
storage/bucket.go Show resolved Hide resolved
@frankyn frankyn requested a review from tritone June 16, 2022 21:25
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jun 17, 2022
tritone
tritone reviewed Jun 17, 2022
View reviewed changes
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.

Some comment verbiage stuff. Also, would you mind adding a case to integration tests as well? Could go here: https://github.com/googleapis/google-cloud-go/blob/main/storage/integration_test.go#L255

storage/bucket.go Outdated Show resolved Hide resolved
@frankyn frankyn added the status: blocked Resolving the issue is dependent on other work. label Jun 17, 2022
@frankyn
Copy link
Member Author

frankyn commented Jun 17, 2022

Thanks @tritone, I'm blocked at the moment. I'm not sure how I can detect if Age is actually returned by the API as 0 or if it's not.

Do you have tips or guidance here? I tried using NullFields but that's only for Requests and not Responses.

@tritone
Copy link
Contributor

tritone commented Jun 17, 2022

Update: conferred with @frankyn and we see that LifecycleRule.Age is implemented as int64 in google-api-go-client as well. Therefore, we can't see from the response whether the server sent the field or not if it was a zero. This means that we could end up corrupting users' lifecycle rules if they use this library to do a get/modify/update loop on their set of rules for a bucket.

Really this field should have been special-cased in the Apiary library as a pointer, but we can't make that change now in Apiary as it would be a breaking change for Apiary users. We're looking into workarounds and should have an update soon.

@frankyn frankyn mentioned this pull request Jun 22, 2022
Merged
@frankyn
Copy link
Member Author

frankyn commented Jun 22, 2022 •
edited by tritone

Current plan to address this issue based on discussion with @tritone and @codyoss:

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Jul 17, 2022
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Aug 16, 2022
tritone added a commit to tritone/google-cloud-go that referenced this pull request Aug 24, 2022
@tritone
chore(storage): bump apiary dependency
9b6ba32 
Bump dependency to pull in pointer field update for Lifecycle
from googleapis/google-api-go-client#1598

Updates googleapis#6204
@tritone tritone mentioned this pull request Aug 24, 2022
Merged
tritone added a commit that referenced this pull request Aug 24, 2022
@tritone
chore(storage): bump apiary dependency (#6562)
ceba364 
Bump dependency to pull in pointer field update for Lifecycle
from googleapis/google-api-go-client#1598

Updates #6204
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Aug 24, 2022
@frankyn frankyn removed status: blocked Resolving the issue is dependent on other work. stale: extraold Pull request is critically old and needs prioritization. labels Aug 24, 2022
@frankyn frankyn requested a review from tritone August 24, 2022 22:50
@product-auto-label product-auto-label bot added the stale: extraold Pull request is critically old and needs prioritization. label Aug 25, 2022
tritone
tritone reviewed Aug 25, 2022
View reviewed changes
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.

As I mentioned before, I'd like to have a case added to the integration tests for this as well, showing that the roundtrip of having allObjects set to/from the server works as expected. Can go here: https://github.com/googleapis/google-cloud-go/blob/main/storage/integration_test.go#L255

storage/bucket.go Outdated
r.Condition.AgeInDays = getAgeCondition(rr.Condition.Age)
if rr.Condition.Age != nil {
r.Condition.AgeInDays = *rr.Condition.Age
if rr.Condition.Age == googleapi.Int64(0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say if *rr.Condition.Age == 0, no need to create another pointer. (Actually I don't know if this works as written, since the pointers will not actually be equal.)

storage/bucket.go Show resolved Hide resolved
codyoss
codyoss approved these changes Aug 25, 2022
View reviewed changes
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a small nit.

storage/bucket.go Outdated Show resolved Hide resolved
tritone
tritone approved these changes Aug 25, 2022
View reviewed changes
storage/bucket.go Show resolved Hide resolved
@tritone tritone enabled auto-merge (squash) August 25, 2022 21:23
@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 26, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 26, 2022
@tritone tritone merged commit c85704f into googleapis:main Aug 26, 2022
@release-please release-please bot mentioned this pull request Aug 26, 2022
Merged
@tritone tritone mentioned this pull request Aug 29, 2022
Closed
@rileykarson rileykarson mentioned this pull request Nov 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tritone tritone tritone approved these changes

@codyoss codyoss codyoss approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. size: m Pull request size is medium. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

storage: re-enable OLM tests once generated library breaking change is released
4 participants
@frankyn @tritone @codyoss @kokoro-team