Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update minimum Go version to 1.19 #4586

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

tianon
Copy link
Contributor

@tianon tianon commented Sep 29, 2023

On Go 1.18 since a5ebe22 (#4226), we get:

# github.com/docker/docker-credential-helpers/client
vendor/github.com/docker/docker-credential-helpers/client/command.go:34:39: programCmd.Environ undefined (type *exec.Cmd has no field or method Environ)
note: module requires Go 1.19
# github.com/docker/cli/cli/connhelper/commandconn
cli/connhelper/commandconn/commandconn.go:71:22: undefined: atomic.Bool
cli/connhelper/commandconn/commandconn.go:76:22: undefined: atomic.Bool
cli/connhelper/commandconn/commandconn.go:77:22: undefined: atomic.Bool
cli/connhelper/commandconn/commandconn.go:78:22: undefined: atomic.Bool

These go away when building against 1.19+.

(See also https://github.com/docker/cli/pull/4226/files#r1340556063)

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023

Codecov Report

Merging #4586 (0f59f04) into master (162e490) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4586   +/-   ##
=======================================
  Coverage   59.70%   59.70%           
=======================================
  Files         288      288           
  Lines       24816    24816           
=======================================
  Hits        14817    14817           
  Misses       9113     9113           
  Partials      886      886           

@thaJeztah
Copy link
Member

❤️ can you also update the vendor script?

go 1.18

@tianon
Copy link
Contributor Author

tianon commented Sep 29, 2023

Oh, given #4395, should this technically be backported too?
(I personally only actually care about it on the master branch, not 24 😅)

@thaJeztah
Copy link
Member

Yup; or at least; we accepted the backport, so I think this should also be backported (I added a cherry-pick label)

On Go 1.18 since a5ebe22, we get:

    # github.com/docker/docker-credential-helpers/client
    vendor/github.com/docker/docker-credential-helpers/client/command.go:34:39: programCmd.Environ undefined (type *exec.Cmd has no field or method Environ)
    note: module requires Go 1.19
    # github.com/docker/cli/cli/connhelper/commandconn
    cli/connhelper/commandconn/commandconn.go:71:22: undefined: atomic.Bool
    cli/connhelper/commandconn/commandconn.go:76:22: undefined: atomic.Bool
    cli/connhelper/commandconn/commandconn.go:77:22: undefined: atomic.Bool
    cli/connhelper/commandconn/commandconn.go:78:22: undefined: atomic.Bool

These go away when building against 1.19+.

Signed-off-by: Tianon Gravi <[email protected]>
@tianon
Copy link
Contributor Author

tianon commented Sep 29, 2023

❤️ can you also update the vendor script?

go 1.18

Updated (including the other reference there to 1.18, and re-ran it with ./scripts/vendor update to verify that nothing actually changes).

@thaJeztah
Copy link
Member

Thanks! I already had some cherry-picks for go1.21 in #4583, and I'll move those to a separate PR (everything except for the go1.21 update itself); I'll include a cherry-pick of this one in that.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

EOL
}

update() {
(set -x ; go mod tidy -compat=1.18 -modfile=vendor.mod; go mod vendor -modfile=vendor.mod)
(set -x ; go mod tidy -compat=1.19 -modfile=vendor.mod; go mod vendor -modfile=vendor.mod)
Copy link
Member

Choose a reason for hiding this comment

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

honestly wondering if we still need this one (I recall it was more important between some older versions that didn't include the // indirect block), but I guess it does not harm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to change anything if I remove it - want me to commit that?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, I think it's fine to keep it for now, and be on the safe side in case they decide to change the format in an incompatible way.

@thaJeztah thaJeztah merged commit 05bec8d into docker:master Sep 29, 2023
74 checks passed
@tianon tianon deleted the go1.19min branch September 29, 2023 08:00
@thaJeztah
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants