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

[WIP] Load defaults from versions.yaml for podvm build #1057

Closed

Conversation

tumberino
Copy link
Contributor

Built off of #1018

  • Renamed existing Makefile to Makefile.image
  • Refactored existing Makefile to target Docker image build process
  • Created Makefile.defaults to load the required values from versions.yaml

Files Updated (Excluding changes from 1018)

  • hack/yq.sh
  • podvm/Makefile
  • podvm/Makefile.image
  • podvm/Makefile.default

yoheiueda and others added 2 commits May 30, 2023 16:44
Signed-off-by: Yohei Ueda <yohei@jp.ibm.com>
@tumberino
Copy link
Contributor Author

@yoheiueda could you take a look at the Makefile changes (excluding the Makefile.defaults for now) and see if it is worth adding into your #1018 ?

@yoheiueda
Copy link
Member

@jtumber-ibm Your proposed change looks good to me.

With your change, we can build a builder image from the remote repo based on centos by this.

make docker-builder-remote-centos

And, we can build a builder image from the remote repo based on centos by this.

make docker-podvm-local-ubuntu

One thing I noticed is that my proposed Dockefile refactoring requires docker buildx, so I think it is better to use docker buildx build explicitly.

Another point is that how we can support podman build in addition to docker.

So, is it possible to parameterize the container image build command like this?

BUILD_CMD = docker buildx build

Then, we can change the build command like this.

make BUILD_CMD='podman build' build-podvm-local-ubuntu

@bpradipt any comments?

@yoheiueda
Copy link
Member

I think the Makefile.defaults part is work-in-progress, and you are possibly working on a similar idea, but I think we can define a fuction like below to reduce redundancy. (I don't come up with a good function name though.)

define query
	$(or $(shell $(YQ_PATH) '.$(1) | select(. != null)' $(VERSIONS_SRC)), \
	     $(error $(1) is not set, and could not be automatically set from $(VERSIONS_SRC)))
endef

ubuntu_amd64_IMAGE_URL ?= $(call query,cloudimg.ubuntu.focal.amd64.url)

@tumberino
Copy link
Contributor Author

@yoheiueda from a quick test it seems that podman doesn't support only building the required stages

@yoheiueda
Copy link
Member

Thank you for the investigation! Probably, we can support docker buildx only for the time being.

Comment on lines +23 to +24
wget -q https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/${BINARY} -O "$EXE"
chmod +x "$EXE"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO downloading executables transparently isn't a good practice. Maybe we could warn that something required is missing instead?

Comment on lines +25 to +31
# Defaults to Ubuntu Focal amd64 release image. These variables can be overriden as needed
ARG UBUNTU_IMAGE_URL=https://cloud-images.ubuntu.com/releases/focal/release-20230107/ubuntu-20.04-server-cloudimg-amd64.img
ARG UBUNTU_IMAGE_CHECKSUM=3895e38566e5c2c019f5c6f825ab7570ee34dac6b9142fab0c7e5a78084c4280

# Defaults to CentOS 8-stream x86_64 image. These variables can be overriden as needed
ARG CENTOS_IMAGE_URL=https://cloud.centos.org/centos/8-stream/x86_64/images/CentOS-Stream-GenericCloud-8-20220913.0.x86_64.qcow2
ARG CENTOS_IMAGE_CHECKSUM=8717251f8e4d2fe3e5032799caae89358c1ba68d65a16b5128a59ec6003aac1c
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could improve reproducibility and traceability of these build if we don't set any defaults at all. The source of truth should be the versions.yaml file, right?

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.

3 participants