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

go.mod: docker 5aac513617f072b15322b147052cbda0d451d389 / v22.06-dev #9940

Merged
merged 1 commit into from Oct 21, 2022

Conversation

thaJeztah
Copy link
Member

This restores compatibility with go1.18, which was broken since commit; moby/moby@c062238

cmd.Environ() is new in go1.19, and not needed for this specific case. Without this, trying to use this package in code that uses go1.18 will fail;

builder/remotecontext/git/gitutils.go:216:23: cmd.Environ undefined (type *exec.Cmd has no field or method Environ)

Changing to use os.Environ() instead restores compatibility with go1.18

Full diff: moby/moby@f9cb47a...5aac513

Related issue

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

This restores compatibility with go1.18, which was broken since commit;
moby/moby@c062238

cmd.Environ() is new in go1.19, and not needed for this specific case.
Without this, trying to use this package in code that uses go1.18 will fail;

    builder/remotecontext/git/gitutils.go:216:23: cmd.Environ undefined (type *exec.Cmd has no field or method Environ)

Changing to use `os.Environ()` instead restores compatibility with go1.18

Full diff: moby/moby@f9cb47a...5aac513

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@milas @StefanScherer @glours PTAL 🤗

@codecov-commenter
Copy link

Codecov Report

Base: 74.56% // Head: 74.56% // No change to project coverage 👍

Coverage data is based on head (533abc3) compared to base (e8ea3ad).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##               v2    #9940   +/-   ##
=======================================
  Coverage   74.56%   74.56%           
=======================================
  Files           2        2           
  Lines         232      232           
=======================================
  Hits          173      173           
  Misses         51       51           
  Partials        8        8           

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.

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

I'll bring this one in, so that it's the first commit after v2.12.1, keeping our options open 👍

@thaJeztah thaJeztah merged commit f472ce3 into docker:v2 Oct 21, 2022
@thaJeztah thaJeztah deleted the go1.18_compat branch October 21, 2022 19:37
@milas
Copy link
Member

milas commented Oct 21, 2022

FWIW we maintain compatibility with compose-go for both actively supported Go versions (and have CI to that effect), but have explicitly not actively supported docker/compose usage as a library thus far

(Agreed that having the change available is good though, thanks for making the updates here)

@thaJeztah
Copy link
Member Author

@milas yeah, so in this case, it was because compose depends on (not yet released) v22.06 code (through buildx). v22.06 is intended for go1.19 and above, but the current 20.10 release is still on go1.18. The release pipeline uses that version of go to build the binaries (which currently does not allow for differentiation go versions for each individual binary), causing the build to fail; docker/docker-ce-packaging#775

Perhaps we can add some hacks to install multiple go versions in the container build environment (and some PATH tricks to switch versions), but we didn't have it yet, so having this change in gives use options to either patch the build pipeline, or do a 2.12.2 patch release from the commit from this PR.

@milas
Copy link
Member

milas commented Oct 21, 2022

Ah right, docker-ce-packaging 🙈

@thaJeztah
Copy link
Member Author

I know.. I know. Having all tied together.. complicate "things".

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.

None yet

4 participants