-
Notifications
You must be signed in to change notification settings - Fork 799
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
add multi-arch build of upstream skopeo image via Travis #1066
Conversation
729fda6
to
413e163
Compare
@TomSweeneyRedHat @mtrmac @cevich @edsantiago @vrothberg PTAL If this works, this is great. would love to see this for Buildah and Podman as well. Would be nice to get something like this to work for the stable branch. |
@rhatdan thanks for the answer. I've created this PR to get understanding if this approach is acceptable. If it's ok, I can do the same for stable branch. Just want to go step by step :) |
@barthy1 I want multi arch images available in quay.io and to use manifests correctly. So I am fine with this. I want to get feedback @mtrmac @vrothberg @TomSweeneyRedHat though. |
Makefile
Outdated
@@ -82,6 +82,10 @@ ifeq ($(DISABLE_CGO), 1) | |||
override BUILDTAGS = exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp | |||
endif | |||
|
|||
ARCHES = amd64 s390x ppc64le | |||
MA_IMAGE := quay.io/skopeo/$(IMAGE_TYPE):ma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, but we may also want to push to quay.io/containers/skopeo:ma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I believe quay.io/containers/skopeo:ma
should be based on the stable
version, not on the upstream
, which is target of this PR for now?
In general I'd suggest to add one more env variable - EXTRA_REPO
(EXTRA_REPOS?
) and when it exists - retag and push multi-arch image also to the extra quay.io repo(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'd suggest that the quay.io/containers/skope:ma
should be based on the stable
version. Currently, quay.io/containers/skopeo:latest
and quay.io/skopeo/stable:latest
are equivalent.
I love the concept, but unfortunately, I've been buried in other stuff the past couple of days. I do want to dig into this though. |
413e163
to
73072e2
Compare
@TomSweeneyRedHat I've updated the PR, adding image build for stable version and pushing it to |
@TomSweeneyRedHat any suggestions for this PR? |
73072e2
to
b7f2d1b
Compare
Thanks for the ping @barthy1 . @edsantiago or @cevich, could one of you take a looksee at this please? |
I will not have time to look this week. My recollection from last month is that I had a strong averse reaction to the |
@edsantiago thank you for input! I can change tag to any value community prefers to have. |
@barthy1 That works for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience. At first glance it LGTM, but (caveat) this was not a thorough review, and I'm not well versed in Travis anyway.
Did you look for ways to refactor the multiple image-build-push
stanzas? Maybe using with_items
? I worry that the duplication will become unmaintainable.
And yes, using :master
and/or :v-actual-version
sounds ideal to me.
.travis.yml
Outdated
os: osx | ||
install: | ||
# Ideally, the (brew update) should not be necessary and Travis would have fairly | ||
# frequenstly updated OS images; that�~@~Ys not been the case historically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo ("frequently", no "s"). And, weird localization characters. Please make sure your editor is set to UTF-8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, I just copied frequenstly
from existing text :) fixed
566dcc3
to
83003c7
Compare
Iterating via archs is not doable in case when we need to keep order and archs should be applied only to one stage. |
83003c7
to
d2a17e3
Compare
@edsantiago hey! any time this week to review the PR? |
Suggestion 1: Since both the skopeo binary build and the container images follow after package updates, it probably makes sense to enable some cron-triggered (in Travis settings) builds. Otherwise the binaries/images could miss out on important dependency security updates if the pace of PR merges (as written) aren't rapid enough. Suggestion 2: (forgive me if I missed it) It would be helpful to maintainers, if just the "build" steps also ran for PRs (i.e. only skip the push). This way any code/script changes which affect the mechanics will be noticed right away, instead of being delayed to branch-push time. |
Quick note first: the CI failure is not your PR's fault; it is being investigated in #1096. I've been trying to review this today, but it's a big job for me because I don't know anything about Travis. It's now late and my brain is fried; and I'm still not much closer to understanding the full details of this PR. I'm sorry. |
d2a17e3
to
0bcb883
Compare
@cevich thank you for suggestions!
Good point. You can enable Travis cron jobs in your Travis project settings - https://docs.travis-ci.com/user/cron-jobs/ Nothing is configured in the code.
That's just brilliant! You are absolute right, sorry that I didn't do that from the beginning. Now I've added image builds (upstream and stable) to PR pipeline. That's you can see already in Travis setup for my PR - https://travis-ci.org/github/containers/skopeo/builds/741935428 . The push part won't be done, as it's still disabled for PR case. |
Yep, I'm happy to set this up.
You wouldn't be saying that if you knew how many years it took me to arrive at that 😞 Presumably w/o much/any effort, this will also be covered by cron-triggered builds. I believe they simply execute as if branch-push happened, so the full build + push workflow should happen. Sorry my Travis-Foo isn't what it use to be |
That’s definitely useful, OTOH I’m worried about the update-test cycle getting longer; it’s already too long for much comfort. Is there a way to measure the time spent running the tests but not waiting for Travis to queue the first one? Travis reports “total time”, which just seems to sump the duration of all tasks, and probably does not account for parallelism; and a “Ran for” which I can’t interpret (it is now shorter than before, at least in one instance?!), so neither seems to be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m afraid I don’t know much about Travis, so mostly focusing on maintainability over time.
.travis.yml
Outdated
# Dont's push if it's PR check | ||
- if [[ "$TRAVIS_EVENT_TYPE" != "pull_request" ]]; then make push-arch-container ; fi | ||
# skopeo stable image build and push | ||
- export SOURCE_TYPE=stable; export REPO=skopeo/stable; export TAG=v1.2.0; export EXTRA_REPO=containers/skopeo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tag value should, if at all possible, be dynamically computed, or it’s going to stay at v1.2.0 forever.
(Alternatively, create a new “what to do on a release” document or script, but there’s high risk it will not be always followed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a little bit tricky. as we have several architectures (and builds) + multi-arch manifest creation after. In general it's possible for image-build-push
step, I mean just build container image and run it to get the version, but not doable for multi-arch-manifest-push
as no build is done there and no container image is available to check the version.
I've follow the second option - with “what to do on a release” document, thank you for input
Makefile
Outdated
push-ma-manifest: | ||
echo "${QUAY_PASSWORD}" | docker login ${REGISTRY} -u "${QUAY_USERNAME}" --password-stdin | ||
sudo chmod 0755 /etc/docker | ||
DOCKER_CLI_EXPERIMENTAL="enabled" docker manifest create "${MA_IMAGE}" $(foreach arch,${ARCHES}, ${MA_IMAGE}-${arch}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Podman experts, should we be using Docker for all of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker is default container tool for travis. podman and buildah, as far as I know, are questionable to install with some hacks (especially for non amd64 archs), see https://travis-ci.community/t/podman-libpod-support/6823/6. I can play with it, if it's hard requirement, however would prefer to use docker, as it's more stable and verified approach for Travis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer podman, but @barthy1 is right, travis at this point defaults pretty hard to docker from what I've seen. @edsantiago might have other thoughts.
Travis is used, as it has native hardware to run the build for many architectures (amd64, s390x, ppc64le). Docker is used as build and manifest tool. `quay.io/skopeo/upstream:master`, `quay.io/skopeo/stable:v1.2.0` and `quay.io/containers/skopeo:v1.2.0` are specified as target multi-arch upstream image. Travis config file has 3 stages: - local-build to do the local test for linux/amd64 and osx, as it was in the initial code - image-build-push to build and push images for specific architectures (amd64, s390x, ppc64le) - manifest-multiarch-push to create and push manifest for multi-arch image - `quay.io/skopeo/upstream:master`, `quay.io/skopeo/stable:v1.2.0` and `quay.io/containers/skopeo:v1.2.0` last stage amnd image push step are not done for pull request. 2 env variables specified in Travis settings are expected - QUAY_USERNAME and QUAY_PASSWORD to push the images to quay.io. As a result multi-arch images for 3 architectures are published. README about build setup id prepared Signed-off-by: Yulia Gaponenko <[email protected]>
0bcb883
to
39f8117
Compare
I am going to merge, and we can look at the Podman versus Docker in a different PR. |
@rhatdan thank you for merging! Don't forget to specify QUAY_USERNAME and QUAY_PASSWORD in Travis settings for this repo -> https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings |
@TomSweeneyRedHat Do you know what these are? |
Just in case - |
@rhatdan I know what they are, I'll see if I can set them... |
...rats. Two major problems:
|
...double checked. Indeed, the robot accounts are all namespaced to the user or orginization account that owns them. There's no way around this 😢 i.e. no matter what "name" I give the robot, quay will create I'll open up a PR to split |
Opened #1104 |
Lol @TomSweeneyRedHat okay, why not. Yes, I'm happy to manage this stuff as I've got my fingers in all the automation pies anyway. Yummy. |
This PR is connected to #1010 to get skopeo multi-arch container images.
To build multi-arch upstream container image Travis is used, as it has native hardware to run the build for many architectures (amd64, s390x, ppc64le). Docker is used as build and manifest tool.
quay.io/skopeo/upstream:master
,quay.io/skopeo/stable:v1.2.0
andquay.io/containers/skopeo:v1.2.0
are specified as target multi-arch images.Travis config file has 3 stages:
local-build
to do the local binary build and test for linux/amd64 and osx, as it was inthe initial code
image-build-push
to build and push images for specific architectures (amd64, s390x, ppc64le)multi-arch-manifest-push
to create and push manifest for multi-arch image -quay.io/skopeo/upstream:master
,quay.io/skopeo/stable:v1.2.0
andquay.io/containers/skopeo:v1.2.0
2 last stages are not done for pull request event or for non-master branch.
2 env variables in Travis are expected - QUAY_USERNAME and QUAY_PASSWORD to push the images to quay.io.
As a result multi-arch image for 3 architectures is published.
The code is tested using another quay registry to push https://travis-ci.com/github/barthy1/skopeo/builds/194396214
Signed-off-by: Yulia Gaponenko [email protected]