Skip to content

Use golang 1.17#8865

Closed
rumpl wants to merge 1 commit into
docker:v2from
rumpl:golang-1.17
Closed

Use golang 1.17#8865
rumpl wants to merge 1 commit into
docker:v2from
rumpl:golang-1.17

Conversation

@rumpl
Copy link
Copy Markdown
Member

@rumpl rumpl commented Nov 2, 2021

What I did

Updated golang version to 1.17

Updated some dependencies too, left untouched: containerd, buildx and buildkit, the build fails with buildkit 0.9.1, I would need more time to see what is going on there

Output of check-dependencies after this change:

❯ make check-dependencies     
go list -u -m -f '{{if not .Indirect}}{{if .Update}}{{.}}{{end}}{{end}}' all
github.com/containerd/containerd v1.5.5 [v1.5.7]
github.com/docker/buildx v0.5.2-0.20210422185057-908a856079fc [v0.6.3]
github.com/moby/buildkit v0.8.2-0.20210401015549-df49b648c8bf [v0.9.1]

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

Updated some dependencies, left untouched: containerd, buildkit

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Copy link
Copy Markdown
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread go.mod
github.com/cnabio/cnab-to-oci v0.3.1-beta1
github.com/compose-spec/compose-go v1.0.4
github.com/containerd/console v1.0.2
github.com/containerd/console v1.0.3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we're also updating various dependencies here; were these needed for Go 1.17? (otherwise it's good practice to do this separately).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not needed, thought I could do both at once with a nice comment on the PR, can separate if needed

Comment thread go.mod
module github.com/docker/compose/v2

go 1.16
go 1.17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This may also print a warning that Go 1.17 is the minimum version supported; probably ok (not sure if anything is using the modules from this repository as a dependency), but it's common to support at least "latest" + "latest-1"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe we should put a warning in the README, tell people not to depend on this cli, or just move everything to internal

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps just a warning would be ok. internal always feels a bit like a "big hammer" to me (we forbid you to use this code!)

That said; trying to only export things that are relatively / considered stable is good practice (also helps the mental overhead to know ("this function/const/variable is not exported, so I can change this without side-effects". My IDE also is better at pointing me at unused functions if they're not exported)

Comment thread go.mod
Comment on lines +41 to +44
require (
github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
github.com/Masterminds/semver v1.5.0 // indirect
github.com/Microsoft/go-winio v0.4.17 // indirect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we know if there's any "overrides" in the list of // indirect dependencies? Overall I think the go1.17 format makes sense for "indirect dependencies that we didn't manage", but if there's any // indirect that was explicitly added to the list to force a "newer version than go.mod resolved automatically", it may be good to put those in the first list.

That way it's clear what versions are "managed" in this go.mod (first block), and what versions are set "automatically" (so can be "ignored" when reading the go.mod)

Also see moby/buildkit#2331 (comment)

Copy link
Copy Markdown
Member Author

@rumpl rumpl Nov 2, 2021

Choose a reason for hiding this comment

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

I don't think we manually override any // indirect dependencies, I didn't even know you could do that, I tend to think of the go.mod file as a lockfile I shouldn't update manually but only use go tool (unless you want to change the go version... go modules...)

It's really optimistic in my opinion to update the go.mod file like that, what will happen when go 1.18 or 19 or 20 comes and they decide they want to change the way they write this mod file, like what they did with the 1.17 version

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we manually override any // indirect dependencies, I didn't even know you could do that

Yes, so you can use it in situations where you know an indirect dependency should be on a newer version than what go.mod automatically finds. In that case, you can add a // indirect with the version you want it to be on. The // indirect will automatically disappear once any other dependency requires a newer (or same) version of that dependency.

I know we had a couple of those in buildkit, but if that's not the case here, then no worries

It's really optimistic in my opinion to update the go.mod file like that, what will happen when go 1.18 or 19 or 20 comes and they decide they want to change the way they write this mod file, like what they did with the 1.17 version

I think even for Go 1.17, the split format is still optional (you can give options to split or not), but time will tell of course (it should still be possible to leave comments after the // indirect, e.g.

github.com/foo/bar     // indirect // using a newer version to address #123454

@rumpl
Copy link
Copy Markdown
Member Author

rumpl commented Nov 3, 2021

Closing this one, I'll open two PRs one for golang and one for updating deps

@rumpl rumpl closed this Nov 3, 2021
@rumpl rumpl deleted the golang-1.17 branch November 3, 2021 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants