-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added support for s390x in che.properties for workspace #19195
Added support for s390x in che.properties for workspace #19195
Conversation
Can one of the admins verify this patch? |
df515ba
to
bcfab73
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.
Did not tested but it seems reasonable.
@@ -356,7 +356,7 @@ che.infra.kubernetes.pvc.storage_class_name= | |||
che.infra.kubernetes.pvc.quantity=10Gi | |||
|
|||
# Pod that is launched when performing persistent volume claim maintenance jobs on OpenShift | |||
che.infra.kubernetes.pvc.jobs.image=centos:centos7 | |||
che.infra.kubernetes.pvc.jobs.image=registry.access.redhat.com/ubi8-minimal |
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.
Maybe it's better to fix version instead of relying on the latest tag, which even is not set explicitly
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.
any reason to not use alpine there ?
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.
@sleshchenko Sure will test ubi8 image with fix version and rebase the PR.
@benoitf We thought ubi8-minimal as best suited replacement for centos7. Hence verified build with the same image.
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.
@sleshchenko Rebased PR. Please check.
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.
what about a more strict version registry.access.redhat.com/ubi8-minimal:8.3-230
CC @nickboldt
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.
@skabashnyuk Verified Che deployment with above mentioned image and it worked fine for s390x. Should I rebase PR with above image?
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. please.
CC @nickboldt
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.
@skabashnyuk Rebased PR. Please check.
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.
+1 for UBI. Why make testing harder by having one image in upstream and a different one downstream?
bcfab73
to
ed91aaf
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
@@ -362,7 +362,7 @@ che.infra.kubernetes.pvc.storage_class_name= | |||
che.infra.kubernetes.pvc.quantity=10Gi | |||
|
|||
# Pod that is launched when performing persistent volume claim maintenance jobs on OpenShift | |||
che.infra.kubernetes.pvc.jobs.image=centos:centos7 |
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 still moving tag, but I think it's OK
CRW will fix it on its end as it currently does I think
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.
WIP to generate PRs at intervals to keep Che image refs up to date in #18831
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.
While I'm not so against this PR, I wonder how will this affect if someone just wanted to use this on their S390X cluster because IIRC access to Red Hat Registry requires to be authenticated.
Otherwise, 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.
+1 for language
@sleshchenko any update on this? |
I would love to see approval from platform team who are in charge of Che Server. |
Signed-off-by: Aditi Jadhav <aditi.jadhav@ibm.com>
a72d40c
to
7849126
Compare
UBI images do not require authentication -- they're free to use w/o login. Che operator already uses ubi-minimal, see:
|
dependabot also provide automatic PR about base images |
@aditijadhav38 I would like to discuss at on next |
removing centos7 is good so either alpine or ubi8 is a separated discussion |
What does this PR do?
This PR will add multiarch support to workspace file che.properties because centos:centos7 image is not available for s390x.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
How to test this PR?
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.