[release/2.8] bump up golang version #3812

wy65701436 · 2022-12-02T09:44:22Z

upgrade go version to v1.18.8

Signed-off-by: Wang Yan wangyan@vmware.com

codecov-commenter · 2022-12-02T09:56:25Z

Codecov Report

Base: 58.72% // Head: 58.71% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (aec0dcc) compared to base (dc5b207).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff               @@
##           release/2.8    #3812      +/-   ##
===============================================
- Coverage        58.72%   58.71%   -0.02%     
===============================================
  Files              102      102              
  Lines             7104     7102       -2     
===============================================
- Hits              4172     4170       -2     
  Misses            2286     2286              
  Partials           646      646

Impacted Files	Coverage Δ
src/github.com/docker/distribution/context/http.go	`62.79% <0.00%> (-0.57%)`	⬇️
....com/docker/distribution/notifications/listener.go	`48.42% <0.00%> (ø)`
.../docker/distribution/registry/client/repository.go	`55.68% <0.00%> (ø)`
.../docker/distribution/registry/storage/blobstore.go	`57.31% <0.00%> (ø)`
...docker/distribution/registry/handlers/basicauth.go	`100.00% <0.00%> (ø)`
...r/distribution/registry/storage/linkedblobstore.go	`66.66% <0.00%> (ø)`
...tribution/registry/storage/blobwriter_resumable.go	`50.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

thaJeztah

just some quick comments (haven't looked at the code changes yet)

.github/workflows/ci.yml

Dockerfile

registry/client/transport/http_reader.go

thaJeztah

Thanks! Looks like there's still some unrelated changes and changes not in main in here; I left comments inline

thaJeztah · 2022-12-03T15:55:56Z

registry/storage/driver/inmemory/mfs.go

+	if offset >= int64(len(f.data)) {
+		return 0, io.EOF
+	}


This looks like it's not directly related to the Go update, or if it's needed, should be in it's own commit (cherry-pick); coming from

Fix panic in inmemory driver #3615

Backporting that fix is good, but should not be stuffed in together with a go update (if it's a separate PR, we can also point to it in the release notes)

this must be in, otherwise one UT will panic as mentioned in #3614

For sure, the fix is good, but it's not directly related to go1.18? My issue is mainly that stuffing bug fixes into a commit that only mentions "update go version" hides them.

This makes it hard to find back fixes;

When reading either git log

When reading the release notes

When trying to find out if TestFileReaderSeek fails on go1.18 #3614 made it into a release (and if so, which one)

This is not directly related to Go 1.18, but without this change, UT would have failed.

If I first try to pick a fix, like #3815, there are some faults on Go 1.16 that I don't think are worth investigating.

@wy65701436 do you want to rebase now that we've merged #3815

thaJeztah · 2022-12-03T16:00:55Z

registry/client/repository.go

-		for cnt := range ctlg.Repositories {
-			entries[cnt] = ctlg.Repositories[cnt]
-		}
+		copy(entries, ctlg.Repositories)


This looks unrelated as well; looks like it was part of fbdfd1a; which is

Use http.NewRequestWithContext for outgoing HTTP requests #3711

Not sure if we want to backport that as a whole (it seems rather large), but if we want, should be separate.

copy(entries, ctlg.Repositories) is to fix a goci-lint failure, it must be in.

My approach for these is usually to update golangci-lint first, and any changes related to updated linters together with that update (same PR, depending on the number of fixes needed, same commit). This makes it transparent that updates were needed due to stricter linters.

Golang updates (besides the go install / @version changes, and in some cases replacing deprecated functions) should not require changes to the code; if they do, Go broke their Go 1 compatibility contract, which should be reported. Keeping the changes related to the Go update itself together, shows "this is what was needed to update go" (which should be "only update the version")

So, in general, you would want us to fix all the prerequisites for the go upgrade, and then have this pr focus only on the go version upgrade.

i understand, but, it's not easy to decouple the fix for golangci-lint from the version upgrade of go, because there is some correlation between the versions of go and golangci-lint. Put all these changes together because they are what is needed for the upgraded version from the point of view of code changes.

releases/v2.8.1.toml

thaJeztah · 2022-12-03T16:04:40Z

version/version.sh

-// used if the registry is run after a go get based install.
+// used if the registry is run after a go install based install.


Looks like we didn't update this in the main branch; we should do it there as well;
https://github.com/distribution/distribution/blob/92d136e113cf34fcfd5f0c21fadf6eac2f39e803/version/version.sh#LL20

will raise pr on main later.

script/validate/dco

BUILDING.md

thaJeztah · 2022-12-03T16:24:13Z

context/http.go

-		if strings.HasPrefix(keyStr, "vars.") {
-			keyStr = strings.TrimPrefix(keyStr, "vars.")
-		}
-
-		if v, ok := ctx.vars[keyStr]; ok {
+		if v, ok := ctx.vars[strings.TrimPrefix(keyStr, "vars.")]; ok {


This change is unrelated, and looks to be part of 3b391d3;

replace strings.Split(N) for strings.Cut() or alternatives #3769

I suggest reverting this; if we want the other changes, we should backport it as a whole and having this partial change will complicate that (result in merge conflicts)

again, it's to fix a goci-lint failure.

upgrade go version to v1.18.8 Signed-off-by: Wang Yan <wangyan@vmware.com>

milosgajdos · 2023-04-17T10:06:38Z

Ping @thaJeztah

thaJeztah · 2023-05-09T09:04:10Z

I had a look at the changes included in this PR, and where they came from; opened an alternative with those changes applied / cherry-picked separately (separate commits), and rebased this PR on top to check for the diff; see #3903

milosgajdos · 2023-05-09T18:20:18Z

Closing, #3908 switched 2.8.X to Go 1.19

wy65701436 mentioned this pull request Dec 2, 2022

bump go 1.18 #3808

Closed

wy65701436 requested review from thaJeztah and milosgajdos December 2, 2022 09:51

thaJeztah changed the title ~~bump up golang version~~ [release/2.8] bump up golang version Dec 2, 2022

thaJeztah reviewed Dec 2, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

wy65701436 force-pushed the 2.8-go-1.18.8 branch from aec0dcc to 8f43012 Compare December 2, 2022 10:22

milosgajdos requested a review from thaJeztah December 2, 2022 10:47

thaJeztah reviewed Dec 2, 2022

View reviewed changes

registry/client/transport/http_reader.go Outdated Show resolved Hide resolved

wy65701436 force-pushed the 2.8-go-1.18.8 branch 3 times, most recently from f4822b4 to 994987c Compare December 2, 2022 12:54

milosgajdos approved these changes Dec 2, 2022

View reviewed changes

milosgajdos requested a review from thaJeztah December 2, 2022 20:40

thaJeztah requested changes Dec 3, 2022

View reviewed changes

milosgajdos requested a review from thaJeztah December 4, 2022 13:30

wy65701436 force-pushed the 2.8-go-1.18.8 branch from 994987c to d0d4c22 Compare December 4, 2022 13:31

bump up golang version

325a6f1

upgrade go version to v1.18.8 Signed-off-by: Wang Yan <wangyan@vmware.com>

wy65701436 force-pushed the 2.8-go-1.18.8 branch from d0d4c22 to 325a6f1 Compare December 4, 2022 13:34

tristanmorgan mentioned this pull request Jan 27, 2023

New Release ? #3809

Closed

thaJeztah mentioned this pull request May 9, 2023

[release/2.8] bump up golang version (alternative) #3903

Merged

milosgajdos closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/2.8] bump up golang version #3812

[release/2.8] bump up golang version #3812

wy65701436 commented Dec 2, 2022

codecov-commenter commented Dec 2, 2022

thaJeztah left a comment

thaJeztah left a comment

thaJeztah Dec 3, 2022

wy65701436 Dec 4, 2022 •

edited

thaJeztah Dec 4, 2022

wy65701436 Dec 4, 2022 •

edited

milosgajdos Apr 26, 2023

thaJeztah Dec 3, 2022

wy65701436 Dec 4, 2022 •

edited

thaJeztah Dec 4, 2022

wy65701436 Dec 4, 2022 •

edited

thaJeztah Dec 3, 2022

wy65701436 Dec 4, 2022

thaJeztah Dec 3, 2022

wy65701436 Dec 4, 2022

milosgajdos commented Apr 17, 2023

thaJeztah commented May 9, 2023

milosgajdos commented May 9, 2023

		// used if the registry is run after a go get based install.
		// used if the registry is run after a go install based install.

[release/2.8] bump up golang version #3812

[release/2.8] bump up golang version #3812

Conversation

wy65701436 commented Dec 2, 2022

codecov-commenter commented Dec 2, 2022

Codecov Report

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wy65701436 Dec 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wy65701436 Dec 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wy65701436 Dec 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wy65701436 Dec 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosgajdos commented Apr 17, 2023

thaJeztah commented May 9, 2023

milosgajdos commented May 9, 2023

wy65701436 Dec 4, 2022 •

edited

wy65701436 Dec 4, 2022 •

edited

wy65701436 Dec 4, 2022 •

edited

wy65701436 Dec 4, 2022 •

edited