Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Support selecting the release when building Ignite images #193

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jul 16, 2019

Currently release selection is supported for amazonlinux, centos,
ubuntu and the kubeadm build based on ubuntu. To select a release,
use the RELEASE env variable, e.g. WHAT=ubuntu RELEASE=cosmic make.
The default release is latest. Fixes #116.

Currently release selection is supported for `amazonlinux`, `centos`,
`ubuntu` and the `kubeadm` build based on `ubuntu`. To select a release,
use the `RELEASE` env variable, e.g. `WHAT=ubuntu RELEASE=cosmic make`.
The default release is `latest`.
@twelho
Copy link
Contributor Author

twelho commented Jul 16, 2019

By setting VERSION= the Ignite version tag is omitted entirely,
resulting in e.g. weaveworks/ignite-ubuntu:bionic.

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

A couple of changes requested.

FROM weaveworks/ignite-ubuntu:latest
ARG RELEASE

FROM weaveworks/ignite-ubuntu:${RELEASE}
Copy link
Contributor

Choose a reason for hiding this comment

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

for this, always lock it to 18.04; as that's vetted for k8s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

images/Makefile Outdated
docker push ${DOCKER_USER}/ignite-${WHAT}:${VERSION}
docker tag ${DOCKER_USER}/ignite-${WHAT}:${VERSION} ${DOCKER_USER}/ignite-${WHAT}:latest
docker push ${DOCKER_USER}/ignite-${WHAT}:${TAG}
docker tag ${DOCKER_USER}/ignite-${WHAT}:${TAG} ${DOCKER_USER}/ignite-${WHAT}:latest
docker push ${DOCKER_USER}/ignite-${WHAT}:latest

build-all:
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to set RELEASE for all of these to not lose information

Copy link
Contributor

Choose a reason for hiding this comment

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

also, now we could build e.g. both k8s for 18.04, 16.04 and 19.04
reflect this change in the README; and update the ignite-ubuntu:latest strings to ignite-ubuntu:18.04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need to set RELEASE for all of these to not lose information

? Release should always be set, and is set automatically to latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, now we could build e.g. both k8s for 18.04, 16.04 and 19.04
reflect this change in the README; and update the ignite-ubuntu:latest strings to ignite-ubuntu:18.04

Isn't this what's done by the above change request? kubeadm now always uses 18.04.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I meant in these lines below; we run $(MAKE) build WHAT=foo. you need to add RELEASE in there to build the different combinations automatically (e.g. 18.04, 19.04, etc.).

The only thing I as an user should do to build everything is make push-all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK gotcha. Now done, bb35f84 builds 16.04, 18.04 and 19.04 of ubuntu.

images/Makefile Show resolved Hide resolved
@luxas luxas self-assigned this Jul 16, 2019
@luxas luxas added this to the v0.4.2 milestone Jul 16, 2019
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

thanks, now looks good.
the github diff is very odd, however.
two last comments

images/Makefile Outdated
$(MAKE) ${OP} WHAT=alpine
$(MAKE) ${OP} WHAT=ubuntu RELEASE=16.04 IS_LATEST=true
$(MAKE) ${OP} WHAT=ubuntu RELEASE=18.04 IS_LATEST=true
$(MAKE) ${OP} WHAT=ubuntu RELEASE=19.04 IS_LATEST=true
Copy link
Contributor

Choose a reason for hiding this comment

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

there can only be one latest tag. remove IS_LATEST from 19.04 and 16.04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, copy-pasted them a bit too quickly 😄. Done in
91e375e.

images/Makefile Outdated
$(MAKE) ${OP} WHAT=amazon-kernel
$(MAKE) ${OP} WHAT=amazonlinux RELEASE=2 IS_LATEST=true
$(MAKE) ${OP} WHAT=alpine
$(MAKE) ${OP} WHAT=ubuntu RELEASE=16.04 IS_LATEST=true
Copy link
Contributor

Choose a reason for hiding this comment

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

remove IS_LATEST here, 18.04 is the only one with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

18.04 is now the latest of ignite-ubuntu.

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@luxas luxas merged commit ac95fab into master Jul 16, 2019
@luxas luxas deleted the image-releases branch July 16, 2019 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help command could pin to a specific Ubuntu version?
2 participants