chore: bump google cloud storage #3676

bshaaban · 2022-06-28T16:49:05Z

This change updates google cloud dependency as the current version dates back to a 2015 version and does not compile with the latest Go version

closes #3668

Signed-off-by: Bahaiddine A bshaaban@mirantis.com

codecov-commenter · 2022-06-28T18:13:36Z

Codecov Report

Merging #3676 (e1f5c29) into main (3e4f8a0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3676   +/-   ##
=======================================
  Coverage   56.58%   56.58%           
=======================================
  Files         103      103           
  Lines        7520     7520           
=======================================
  Hits         4255     4255           
  Misses       2596     2596           
  Partials      669      669

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 3e4f8a0...e1f5c29. Read the comment docs.

thaJeztah · 2022-06-28T20:49:58Z

Looks like there's an issue with the go.sum not matching the expected result;

#15 [validate 1/1] RUN --mount=target=/context   --mount=target=.,type=tmpfs <<EOT (set -e...)
#15 1.062 ERROR: Vendor result differs. Please vendor your package with "make vendor"
#15 1.233  M go.sum

edit: I tried make vendor locally, and indeed get a diff;

git diff
diff --git a/go.sum b/go.sum
index 27e9e064..fa2d82b6 100644
--- a/go.sum
+++ b/go.sum
@@ -106,7 +106,6 @@ github.com/bugsnag/osext v0.0.0-20130617224835-0dd3f918b21b/go.mod h1:obH5gd0Bsq
 github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0 h1:nvj0OLI3YqYXer/kZD8Ri1aaunCxIEsOst1BVJswV0o=
 github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0/go.mod h1:D/8v3kj0zr8ZAKg1AQ6crr+5VwKN5eIywRkfhyM/+dE=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
-github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/cespare/xxhash/v2 v2.1.2 h1:YRXhKfTDauu4ajMg1TPgFO5jnlC2HCbmLXMcTG5cbYE=

thaJeztah · 2022-06-28T20:52:46Z

and does not compile with the latest Go version

I'm curious though; do you have more details about this? I see CI on this repository is using Go 1.18; is it failing on 1.19beta1 or 1.18?

bshaaban · 2022-06-28T21:05:30Z

and does not compile with the latest Go version

I'm curious though; do you have more details about this? I see CI on this repository is using Go 1.18; is it failing on 1.19beta1 or 1.18?

actually i'm still on Go 1.17.10, I believe vendoring might not work the same based on the go version. Locally, make vendor works but make validate-vendor fails for me.

Yes, it seems go.sum gets updated, i haven't pushed an update since validate-vendor still fails for me.

thaJeztah · 2022-06-28T21:55:16Z

Both should be running inside a Dockerfile, so afaics, the host environment should not make a difference;

distribution/Makefile

Lines 106 to 114 in 26a586c

    
           validate-vendor: ## validate vendor 
        
           	docker buildx bake $@ 
        
           vendor: ## update vendor 
        
           	$(eval $@_TMP_OUT := $(shell mktemp -d -t buildx-output.XXXXXXXXXX)) 
        
           	docker buildx bake --set "*.output=$($@_TMP_OUT)" update-vendor 
        
           	rm -rf ./vendor 
        
           	cp -R "$($@_TMP_OUT)"/out/* . 
        
           	rm -rf $($@_TMP_OUT)/*

Could it be you tried to run make validate-vendor before committing the changes?

bshaaban · 2022-06-28T22:26:52Z

Both should be running inside a Dockerfile, so afaics, the host environment should not make a difference;

distribution/Makefile

Lines 106 to 114 in 26a586c

validate-vendor: ## validate vendor

docker buildx bake $@

vendor: ## update vendor

$(eval $@_TMP_OUT := $(shell mktemp -d -t buildx-output.XXXXXXXXXX))

docker buildx bake --set "*.output=$($@_TMP_OUT)" update-vendor

rm -rf ./vendor

cp -R "$($@_TMP_OUT)"/out/* .

rm -rf $($@_TMP_OUT)/*

Could it be you tried to run make validate-vendor before committing the changes?

this is probably right. I haven't run make validate-vendor prior to pushing my change. I've pushed an update to include this go.sum change. Probably additional changes are needed in the code base to update the google cloud storage calls to match their most recent updates/tags.

bshaaban · 2022-06-30T16:04:12Z

It seems the logic in registry/storage/driver/gcs/gcs.go must be refactored to take into account the new google cloud storage api, code compilation is skipped to to the go build tag in the file.

Jamstah · 2022-07-11T15:33:27Z

It seems the logic in registry/storage/driver/gcs/gcs.go must be refactored to take into account the new google cloud storage api, code compilation is skipped to to the go build tag in the file.

Are you saying that there are additional changes required before users can compile for google cloud and use google cloud storage for their registry?

If so, I'd suggest making this a draft PR.

bshaaban · 2022-07-11T16:07:46Z

according to the comments in https://github.com/distribution/distribution/blob/main/registry/storage/driver/gcs/gcs.go#L317-L318, the logic was copied from an outdated google cloud storage api. I believe this logic needs to be updated as well.

Jamstah · 2022-07-11T16:36:23Z

Yes, I don't think the Google cloud storage driver is working. I expect it's not worth merging this PR until the work to fix it is done.

Are you planning to work on it?

bshaaban · 2022-07-11T16:40:41Z

Yes, I don't think the Google cloud storage driver is working. I expect it's not worth merging this PR until the work to fix it is done.

Are you planning to work on it?

i'll have a look and see how to convert current logic with latest api code

thaJeztah · 2022-07-12T12:47:39Z

code compilation is skipped to to the go build tag in the file.

Are there other plugins that are optional (and thus not compiled by default)? If so, we should have a job in CI that at least tries to compile the binaries with those build-flags set (so that we can see when things break)

We should also list the available build-tags (and their meaning) under https://github.com/distribution/distribution/blob/main/BUILDING.md#optional-build-tags

Jamstah · 2022-07-12T13:31:21Z

We should also list the available build-tags (and their meaning) under https://github.com/distribution/distribution/blob/main/BUILDING.md#optional-build-tags

I was thinking of doing that the other day.

See #3687

kaovilai · 2022-07-18T14:24:11Z

registry/storage/driver/gcs/gcs_test.go

@@ -15,7 +15,6 @@ import (
 	"golang.org/x/oauth2"
 	"golang.org/x/oauth2/google"
 	"google.golang.org/api/googleapi"
-	"google.golang.org/cloud/storage"


No replacement storage import here.

kaovilai · 2022-07-26T15:05:27Z

we should have a job in CI that at least tries to compile the binaries with those build-flags set (so that we can see when things break)

@thaJeztah are you opening a PR for this?

kaovilai · 2022-07-26T19:18:37Z

I would be interested in helping with this PR. Let me know if you have something I can help with.

Jamstah · 2022-07-26T22:15:26Z

I would be interested in helping with this PR. Let me know if you have something I can help with.

I think the important thing is getting the storage driver working. Until it's working it's not even worth turning it on in ci or changing dependencies.

jmontleon · 2022-07-28T19:17:11Z

I have a PR started at kaovilai#1

It's a mess, needs cleanup, but the only thing failing at the moment is:

----------------------------------------------------------------------
FAIL: /home/jason/Documents/src/github.com/konveyor/distribution/registry/storage/driver/testsuites/testsuites.go:395: DriverSuite.TestContinueStreamAppendSmall

/home/jason/Documents/src/github.com/konveyor/distribution/registry/storage/driver/testsuites/testsuites.go:396:
    suite.testContinueStreamAppend(c, int64(32))
/home/jason/Documents/src/github.com/konveyor/distribution/registry/storage/driver/testsuites/testsuites.go:419:
    c.Assert(curSize, check.Equals, int64(len(contentsChunk1)))
... obtained int64 = 0
... expected int64 = 32

Trying to figure out what's going wrong, but not really certain atm.

kaovilai · 2022-07-29T02:50:19Z

Keep an eye on #3702

milosgajdos

LGTM. @thaJeztah can you double check if I missed anything, please 🙇

milosgajdos · 2022-07-29T17:22:02Z

@bshaaban do you want to remove WIP from the title of this PR or do you still consider it to be a WIP?

kaovilai · 2022-07-29T17:33:58Z

@milosgajdos I don't think this PR is doing anything. Merging this will still result in broken, non-compiling GCS code.
So maybe close this and merge #3702 instead?

The CI is passing because BUILDTAGS hasn't been passed to go build as demonstrated by CI failure in #3703

kaovilai · 2022-07-29T17:35:49Z

This PR is still WIP per comment

milosgajdos · 2022-07-29T17:39:24Z

Will mark this as draft.

bshaaban · 2022-08-02T17:57:57Z

I can rebase on top of #3702 to maintain dep updates since gcs was updated there already.

This change updates google cloud dependency as the current version dates back to a 2015 version and does not compile with the latest Go version closes distribution#3668 Signed-off-by: Bahaiddine A <bshaaban@mirantis.com>

bshaaban · 2022-08-08T14:17:26Z

closing this pr, in favor of #3702

milosgajdos requested a review from thaJeztah June 28, 2022 18:07

bshaaban force-pushed the bump_google_cloud branch from bd310f2 to e1f5c29 Compare June 28, 2022 22:12

kaovilai mentioned this pull request Jul 11, 2022

OADP-602: Include GCS storage driver when building plugin openshift/openshift-velero-plugin#151

Merged

bshaaban changed the title ~~chore: bump google cloud storage~~ WIP/DONOTMERGE - chore: bump google cloud storage Jul 11, 2022

thaJeztah mentioned this pull request Jul 12, 2022

Add build tags to BUILDING.md #3687

Merged

kaovilai reviewed Jul 18, 2022

View reviewed changes

jmontleon mentioned this pull request Jul 29, 2022

update google.golang.org/cloud #3668

Closed

jmontleon mentioned this pull request Jul 29, 2022

Update GCS dependencies and fix GCS driver #3702

Closed

milosgajdos approved these changes Jul 29, 2022

View reviewed changes

milosgajdos marked this pull request as draft July 29, 2022 17:39

chore: bump google cloud storage

b415ab0

This change updates google cloud dependency as the current version dates back to a 2015 version and does not compile with the latest Go version closes distribution#3668 Signed-off-by: Bahaiddine A <bshaaban@mirantis.com>

bshaaban force-pushed the bump_google_cloud branch from c4767c2 to b415ab0 Compare August 5, 2022 17:50

bshaaban changed the title ~~WIP/DONOTMERGE - chore: bump google cloud storage~~ chore: bump google cloud storage Aug 5, 2022

bshaaban marked this pull request as ready for review August 5, 2022 19:45

bshaaban closed this Aug 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: bump google cloud storage #3676

chore: bump google cloud storage #3676

bshaaban commented Jun 28, 2022

codecov-commenter commented Jun 28, 2022 •

edited

thaJeztah commented Jun 28, 2022 •

edited

thaJeztah commented Jun 28, 2022

bshaaban commented Jun 28, 2022 •

edited

thaJeztah commented Jun 28, 2022

bshaaban commented Jun 28, 2022

bshaaban commented Jun 30, 2022

Jamstah commented Jul 11, 2022

bshaaban commented Jul 11, 2022

Jamstah commented Jul 11, 2022

bshaaban commented Jul 11, 2022

thaJeztah commented Jul 12, 2022

Jamstah commented Jul 12, 2022 •

edited

kaovilai Jul 18, 2022

kaovilai commented Jul 26, 2022

kaovilai commented Jul 26, 2022

Jamstah commented Jul 26, 2022

jmontleon commented Jul 28, 2022 •

edited

kaovilai commented Jul 29, 2022

milosgajdos left a comment

milosgajdos commented Jul 29, 2022

kaovilai commented Jul 29, 2022 •

edited

kaovilai commented Jul 29, 2022

milosgajdos commented Jul 29, 2022

bshaaban commented Aug 2, 2022

bshaaban commented Aug 8, 2022

chore: bump google cloud storage #3676

chore: bump google cloud storage #3676

Conversation

bshaaban commented Jun 28, 2022

codecov-commenter commented Jun 28, 2022 • edited

Codecov Report

thaJeztah commented Jun 28, 2022 • edited

thaJeztah commented Jun 28, 2022

bshaaban commented Jun 28, 2022 • edited

thaJeztah commented Jun 28, 2022

bshaaban commented Jun 28, 2022

bshaaban commented Jun 30, 2022

Jamstah commented Jul 11, 2022

bshaaban commented Jul 11, 2022

Jamstah commented Jul 11, 2022

bshaaban commented Jul 11, 2022

thaJeztah commented Jul 12, 2022

Jamstah commented Jul 12, 2022 • edited

kaovilai Jul 18, 2022

Choose a reason for hiding this comment

kaovilai commented Jul 26, 2022

kaovilai commented Jul 26, 2022

Jamstah commented Jul 26, 2022

jmontleon commented Jul 28, 2022 • edited

kaovilai commented Jul 29, 2022

milosgajdos left a comment

Choose a reason for hiding this comment

milosgajdos commented Jul 29, 2022

kaovilai commented Jul 29, 2022 • edited

kaovilai commented Jul 29, 2022

milosgajdos commented Jul 29, 2022

bshaaban commented Aug 2, 2022

bshaaban commented Aug 8, 2022

codecov-commenter commented Jun 28, 2022 •

edited

thaJeztah commented Jun 28, 2022 •

edited

bshaaban commented Jun 28, 2022 •

edited

Jamstah commented Jul 12, 2022 •

edited

jmontleon commented Jul 28, 2022 •

edited

kaovilai commented Jul 29, 2022 •

edited