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.

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=

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?

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.

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

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

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.

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.

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.

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.

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?

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

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

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

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?

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

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.

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.

Keep an eye on #3702

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

@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

This PR is still WIP per comment

Will mark this as draft.

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

closing this pr, in favor of #3702