Skip to content

Fix gcs storage driver#3929

Merged
milosgajdos merged 4 commits intodistribution:mainfrom
flavianmissi:fix-gcs-oss-file-writer
Jun 1, 2023
Merged

Fix gcs storage driver#3929
milosgajdos merged 4 commits intodistribution:mainfrom
flavianmissi:fix-gcs-oss-file-writer

Conversation

@flavianmissi
Copy link
Contributor

@flavianmissi flavianmissi commented May 23, 2023 •
edited
Loading

Based on #3702.

Also fixes the OSS Cancel method which was missing a context parameter.

@flavianmissi flavianmissi mentioned this pull request May 23, 2023
Closed
@flavianmissi flavianmissi changed the title Fix gcs oss file writer Fix gcs storage driver May 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (8e29e87) 56.88% compared to head (8ffa64a) 56.89%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3929      +/-   ##
==========================================
+ Coverage   56.88%   56.89%   +0.01%     
==========================================
  Files         106      106              
  Lines       10703    10684      -19     
==========================================
- Hits         6088     6079       -9     
+ Misses       3942     3933       -9     
+ Partials      673      672       -1

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@flavianmissi flavianmissi mentioned this pull request May 23, 2023
Closed
milosgajdos
milosgajdos approved these changes May 26, 2023
View reviewed changes
Copy link
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

I'm LGTM-ing this as it's a port of #3702

I dont want this to be blocked for another year by unproductive conversations - we should look for perfection by iterating before v3 is released.

@flavianmissi
Copy link
Contributor Author

flavianmissi commented May 26, 2023

Who can help with the 2nd approval @milosgajdos ?

@milosgajdos
Copy link
Member

milosgajdos commented May 26, 2023

CC: @distribution/maintainers

@flavianmissi
registry/storage/driver: receive context on Cancel methods
d0bc83d 
both oss and gcs driver were missing the context parameter that is
required to satisfy the storagedriver.FileWriter interface.

Signed-off-by: Flavian Missi <[email protected]>
@flavianmissi
registry/storage/driver/gcs: fix code to use updated gcs driver
0207ada 
Signed-off-by: Flavian Missi <[email protected]>
@flavianmissi
go.mod: update gcs driver dependencies
6951028 
Signed-off-by: Flavian Missi <[email protected]>
@flavianmissi
vendor: update gcs driver dependencies files
817dd28 
Signed-off-by: Flavian Missi <[email protected]>
@flavianmissi flavianmissi force-pushed the fix-gcs-oss-file-writer branch from 8ffa64a to 817dd28 Compare May 31, 2023 07:48
@flavianmissi
Copy link
Contributor Author

flavianmissi commented May 31, 2023

bumping this as it's still pending a review.

@milosgajdos milosgajdos requested review from corhere and squizzi May 31, 2023 08:25
@milosgajdos
Copy link
Member

milosgajdos commented May 31, 2023

I've tagged @corhere as he was on the original PR review; added @squizzi too.

squizzi
squizzi approved these changes May 31, 2023
View reviewed changes
Copy link
Collaborator

@squizzi squizzi left a comment

Choose a reason for hiding this comment

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

Based on the discussion in #3702 and the approvals there as well as the minor additional commits added in this to add context to Cancel and bump some gcs dependencies; I feel this is safe to approve.

@milosgajdos milosgajdos merged commit 5902a24 into distribution:main Jun 1, 2023
@dkulchinsky
Copy link

dkulchinsky commented Jun 1, 2023

great to see this getting merged! thanks to all for driving this effort 👏🏼

@milosgajdos
Copy link
Member

milosgajdos commented Jun 1, 2023

Big thanks go to @jmontleon for the initial effort and @flavianmissi for getting it across the finish line 🙇

@jmontleon
Copy link

jmontleon commented Jun 1, 2023

Thank you @flavianmissi and @milosgajdos !

@milosgajdos milosgajdos mentioned this pull request Aug 14, 2023
Closed
@flavianmissi flavianmissi deleted the fix-gcs-oss-file-writer branch December 6, 2023 14:36
@milosgajdos milosgajdos mentioned this pull request Dec 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milosgajdos milosgajdos milosgajdos approved these changes

@squizzi squizzi squizzi approved these changes

@corhere corhere Awaiting requested review from corhere

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@flavianmissi @codecov-commenter @milosgajdos @dkulchinsky @jmontleon @squizzi