-
Notifications
You must be signed in to change notification settings - Fork 79
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
podvm: build rhel peer pod VM qcow2 image for s390x #1531
podvm: build rhel peer pod VM qcow2 image for s390x #1531
Conversation
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.
LGTM
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.
Thanks!
I added some comments, however, while review it seems to me like many changes should have been done in the builder container Dockerfile rather than here.
So I'd generally check what can be moved there (unless i missed something and for some reason you have to do it here)
podvm/Dockerfile.podvm_binaries.rhel
Outdated
@@ -17,7 +17,11 @@ ARG AA_KBC="offline_fs_kbc" | |||
ARG ARCH | |||
ARG CAA_SRC | |||
ARG CAA_SRC_REF | |||
ARG IMAGE_URL="/tmp/rhel.qcow2" | |||
ARG IMAGE_CHECKSUM |
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 don't think it's needed in the binaries dockerfile, only in the podvm one, and it's already being set there, no?
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 required by this line in the binaries dockerfile
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.
@snir911 thanks for the review.
which are required for the make target |
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.
Thanks! LGTM overall add one comment to address
also it seems the latter commit "podvm: align commit with other files" is empty, pls remove it
podvm/Dockerfile.podvm_builder.rhel
Outdated
|
||
# set a correspond qemu-system-* named link to qemu-kvm | ||
# set a corrspond qemu-system-* named link to qemu-kvm |
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.
nit: typo
subscription-manager register --org=${ORG_ID} --activationkey=${ACTIVATION_KEY}; \ | ||
fi | ||
|
||
RUN subscription-manager repos --enable codeready-builder-for-rhel-9-${ARCH}-rpms; \ |
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.
IIUC codeready expects x86_64 rather than amd64, how about treating the inconsistency using the following:
users will set only the ARCH value as done in other files:
ARG ARCH=x86_64 # set x86_64 or s390x
ENV ARCH=${ARCH}
then:
for protoc use: ${ARCH/s390x/s390_64}
for codeready use: ${ARCH}
for golang use: ${ARCH/x86_64/amd64}
for yq use: ${ARCH/x86_64/amd64}
for packer use: ${ARCH}
352bf48
to
282326a
Compare
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.
LGTM Thanks @redobed !
I think some of the commits needs to be squashed
@redobed please squash the commits and also remove the merge commits. |
482636c
to
81a831c
Compare
@bpradipt commit are squashed |
e302e8b
to
5b92633
Compare
This PR updates the podvm dockerfiles to enable building a peerpod QCOW2 image for s390x. It supports building on either a subscribed or an unsubscribed RHEL. Packer for s390x is built from source Signed-off-by: Obed Tetteh <obed.tetteh@ibm.com>
5b92633
to
da2e1ed
Compare
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.
/lgtm
Thanks @redobed
This PR updates the podvm/Dockerfile.podvm_binaries.rhel and podvm/Dockerfile.podvm_builder.rhel to enable building a rhel based peer pod VM image qcow2 image for s390x. Packer binary is conditionally built from source s390x. The PR also allows building the Builder image either on a subscribed or an unsubscribed rhel host.
Signed-off-by: Obed Tetteh obed.tetteh@ibm.com