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

[master] update compose to v2.10.0 #742

Merged
merged 1 commit into from
Aug 20, 2022
Merged

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah changed the title update compose to v2.10.0 [master] update compose to v2.10.0 Aug 19, 2022
@thaJeztah
Copy link
Member Author

@milas @glours PTAL

glours
glours previously approved these changes Aug 19, 2022
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

milas
milas previously approved these changes Aug 19, 2022
Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for bumping this 👍

@thaJeztah
Copy link
Member Author

hmmm... why is this failing CI? Let me check

@thaJeztah
Copy link
Member Author

Ah! Looks to be because of @crazy-max's changes in docker/compose#9744

I was actually discussing that with him at the time, but forgot we needed to make changes 😓

@milas
Copy link
Contributor

milas commented Aug 19, 2022

Ahhh yeah you beat me to it - I'm not really familiar with the docker-ce-packaging setup but am happy to help out making the necessary adjustments if needed!

@thaJeztah
Copy link
Member Author

Yes, I think he overlooked that rpm and deb scripts can't use docker as part of the build, so the builder.Makefile contained the bits to build without docker (the rpm and deb build tools run inside a container, but won't have Docker available).

@thaJeztah
Copy link
Member Author

e.g. for rpm's, this spec runs the build;

pushd ${RPM_BUILD_DIR}/src/compose
# FIXME: using GOPROXY, to work around:
# go: github.com/Azure/[email protected]+incompatible: reading github.com/Azure/azure-sdk-for-go/go.mod at revision v48.2.0: unknown revision v48.2.0
GOPROXY="https://proxy.golang.org" GO111MODULE=on go mod download
GOPROXY="https://proxy.golang.org" GO111MODULE=on GIT_TAG="%{_compose_version}" \
make COMPOSE_BINARY="bin/docker-compose" -f builder.Makefile compose-plugin

and for debs, it the debian "rules" file that runs the build;

# Build the compose plugin
# FIXME: using GOPROXY, to work around:
# go: github.com/Azure/[email protected]+incompatible: reading github.com/Azure/azure-sdk-for-go/go.mod at revision v48.2.0: unknown revision v48.2.0
cd /go/src/github.com/docker/compose \
&& GOPROXY="https://proxy.golang.org" GO111MODULE=on go mod download \
&& mkdir -p /usr/libexec/docker/cli-plugins/ \
&& GOPROXY="https://proxy.golang.org" GO111MODULE=on GIT_TAG=$(COMPOSE_VERSION) \
make COMPOSE_BINARY=/usr/libexec/docker/cli-plugins/docker-compose -f builder.Makefile compose-plugin

@thaJeztah
Copy link
Member Author

I think we created the builder.Makefile to make sure that all the correct variables were set (so that we don't have to replicate that in this repository (at the risk of getting out of sync)

@milas
Copy link
Contributor

milas commented Aug 19, 2022

I opened docker/compose#9771.

Caveat in there:

NOTE: The output path is now bin/build/docker-compose to keep
parity with the rest of the Compose build logic - previously
it was bin/docker-compose (no intermediate build/ dir)

(Sorry, I figured adding slightly more to the one-time pain of getting this sorted was better than getting bitten by slightly different paths the next time we touch the build logic 😓)

Can the packaging be tricked (for the sake of a PR) into using a branch for the ref, or is it smart enough to only operate on for tags/releases? Getting a clean CI pass on this PR using the compose#9771 PR branch and/or docker/compose#v2 once merged would be great to know that it works and then we could tag v2.10.1 and adjust this PR before merging. (Hopefully that made sense)

Regardless, I think we can revisit this on Monday at this point! 👋

@thaJeztah
Copy link
Member Author

I was also eyeing docker/compose#9765 (haven't done a compare yet between your makefile and that one though).

I think we can add some hacks to pull from your fork; I can give that a go.

@thaJeztah thaJeztah dismissed stale reviews from milas and glours via 061b173 August 19, 2022 20:13
@thaJeztah
Copy link
Member Author

I pushed a commit that switches to your fork and uses that commit.

If we don't want to do a new compose release for this, I think we should also be able to add a temporary hack in this repository (where we either copy the instructions, or temporarily add a copy of the makefile

@nicksieger
Copy link
Member

I think it might be ok to just manually force-move the v2.10.0 tag on compose? No code or relevant changes since the placement of the current tag. WDYT?

@thaJeztah
Copy link
Member Author

No! Don't move the tag! It will already be in Go's module proxy, and changing the tag will mark it as tampered with

@milas
Copy link
Contributor

milas commented Aug 19, 2022

Y'know, I'd actually looked at that PR from Ulysses earlier and did not even think about the fact that it's 99.9% the same 🤦

Having a single Makefile with a single local target is probably more sane for everyone 🙃

Sidenote: I just realized that we're technically building Compose with v1.19 for all of its in-repo CI vs 1.18 here 🙈

I'll make note to ensure that:

  • We test the local build task as part of Compose CI
  • We coordinate "major" Go version upgrades with packaging in the future

@thaJeztah
Copy link
Member Author

Sidenote: I just realized that we're technically building Compose with v1.19 for all of its in-repo CI vs 1.18 here 🙈

Ah, yes, and the 20.10 branch (up until earlier today) was still on 1.17. Unfortunately currently the build pipeline is still a "one-size-fits-all" for Go versions. There are a bunch of plans to decouple all of that (more below), but it's currently what we need to deal with.

The docker engine (and to some extend, runc, and containerd) tend to be slightly conservative on Go updates; those parts of our stack use Go in more esoteric corners, and history has shown that if there's some bug in Go hidden in some obscure corner, we usually hit those (ENOENT signals being the last notable one in go 1.13 -> go 1.14 that required literally months of engineering to dig into kernel code and man-pages to evaluate evaluate kernel syscalls to be sure it was safe to upgrade (arghh!). So, depending on changes in go updates, we tend to give new versions some "burn in" time where possible.

We coordinate "major" Go version upgrades with packaging in the future

Thanks! I think in most cases version-1 should not be a big issue (unless you start to use new features on "day 1"). Good practice is to test against "current" and "current - 1" for library code (Go's recommendations), but I realize that's not always possible.

Plans are to decouple the build pipeline more (it has grown to become a bit of a Jenga tower with so many repositories involved), so that compose itself can prepare packages and the pipeline only to be used to signing and publishing them. Still a ton of stuff to do for that, so in the meantime it's a bit "deal with what we have" (sorry for that!).

@thaJeztah
Copy link
Member Author

^^ Good news; at least this patch looks to work. Let me also test it in the 20.10 branch

@thaJeztah thaJeztah marked this pull request as ready for review August 20, 2022 11:40
@thaJeztah
Copy link
Member Author

We merged #744, which fixes the issue; I guess we can decide to keep that, or to switch back to using the builder.Makefile from docker/compose#9771.

Overall, the build for the compose plugin is fairly trivial, but of course the -ldflags is usually where the fun is (so there's still a small risk of diverging)

@thaJeztah
Copy link
Member Author

Whoop, and green; I'll bring this one in.

@thaJeztah thaJeztah merged commit 5275084 into docker:master Aug 20, 2022
@thaJeztah thaJeztah deleted the compose_2.10 branch August 20, 2022 11:56
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.

4 participants