-
Notifications
You must be signed in to change notification settings - Fork 8
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
Switch to CentOS Stream 10 #245
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AshwinHIBM The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
This is probably fine, just want you to confirm whether or not we still need the target substitution in all these places.
@@ -54,25 +54,23 @@ buildContainerd() { | |||
mkdir /workspace/containerd-packaging-${DISTRO} | |||
cp -r /workspace/containerd-packaging-ref/* /workspace/containerd-packaging-${DISTRO} | |||
|
|||
if [[ "${DISTRO_NAME}:${DISTRO_VERS}" == centos:8 ]] | |||
then | |||
if [[ "${DISTRO_NAME}:${DISTRO_VERS}" == centos:8 ]]; then |
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.
we may be able to skip these checks now. iirc the old builds were using the now-deprecated dockerhub images, but it looks like the official build repo has moved to the quay repos. https://github.com/docker/docker-ce-packaging/blob/master/rpm/centos-10/Dockerfile#L6
looking at the script though, maybe this part has to stay as-is. i don't see where that TARGET gets used from a quick look
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 think I know why we still need it. It's best I order it so that we're on the same page :) The most important step would be step 5.
- We get the list of distributions to be built: https://github.com/ppc64le-cloud/docker-ce-build/blob/main/get-env.sh#L69
- We split and take the first part, here, CentOS and store it in DISTRO_NAME https://github.com/ppc64le-cloud/docker-ce-build/blob/main/build-containerd.sh#L48
- We take the second part, here, 10 and store it in DISTRO_VERS https://github.com/ppc64le-cloud/docker-ce-build/blob/main/build-containerd.sh#L49
- Now build containerd: https://github.com/ppc64le-cloud/docker-ce-build/blob/main/build-containerd.sh#L86
- The target will be used here https://github.com/docker/containerd-packaging/blob/main/Makefile#L111
Use CentOS Stream 10 instead of CentOS 10 and get it from quay.io. This PR also includes some minor code style changes.