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.

Sign up for GitHub

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

Jump to bottom

Added support for s390x in che.properties for workspace #19195

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Added support for s390x in che.properties for workspace #19195

merged 1 commit into from
Apr 13, 2021

Conversation

aditijadhav38
Copy link
Contributor

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:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@che-bot
Copy link
Contributor

che-bot commented Mar 3, 2021

Can one of the admins verify this patch?

@che-bot che-bot added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Mar 3, 2021
sleshchenko
sleshchenko approved these changes Mar 3, 2021
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a 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.

sleshchenko
sleshchenko reviewed Mar 3, 2021
View reviewed changes
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties Outdated
@@ -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
Copy link
Member

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

Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@aditijadhav38 aditijadhav38 Apr 2, 2021 •
edited
Loading

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?

Copy link
Contributor

@skabashnyuk skabashnyuk Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yes. please.
CC @nickboldt

Copy link
Contributor Author

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.

Copy link
Contributor

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?

sleshchenko
sleshchenko approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

sleshchenko
sleshchenko reviewed Mar 9, 2021
View reviewed changes
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
@@ -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
Copy link
Member

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

Copy link
Contributor

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

sr229
sr229 approved these changes Mar 28, 2021
View reviewed changes
Copy link
Contributor

@sr229 sr229 left a 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

themr0c
themr0c approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@themr0c themr0c left a comment

Choose a reason for hiding this comment

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

+1 for language

@aditijadhav38
Copy link
Contributor Author

@sleshchenko any update on this?

@sleshchenko
Copy link
Member

I would love to see approval from platform team who are in charge of Che Server.

Added support for s390x in che.properties for workspace
7849126 
Signed-off-by: Aditi Jadhav <aditi.jadhav@ibm.com>
@nickboldt
Copy link
Contributor

nickboldt commented Apr 6, 2021 •
edited
Loading

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.

UBI images do not require authentication -- they're free to use w/o login.

Che operator already uses ubi-minimal, see:

@benoitf
Copy link
Contributor

benoitf commented Apr 6, 2021

dependabot also provide automatic PR about base images

@skabashnyuk
Copy link
Contributor

@aditijadhav38 I would like to discuss at on next Community meeting the idea about common runtime image that touches this pr. At this moment registry.access.redhat.com/ubi8-minimal:8.3-230 looks like one of the candidates with the highest chances. I would like to address all concerns like @benoitf 's before we merge this pr.

@benoitf
Copy link
Contributor

benoitf commented Apr 13, 2021

removing centos7 is good so either alpine or ubi8 is a separated discussion

@skabashnyuk skabashnyuk merged commit aa6bff1 into eclipse-che:master Apr 13, 2021
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 13, 2021
@che-bot che-bot added this to the 7.29 milestone Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf left review comments

@sleshchenko sleshchenko sleshchenko approved these changes

@nickboldt nickboldt nickboldt approved these changes

@themr0c themr0c themr0c approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@sr229 sr229 sr229 approved these changes

@ibuziuk ibuziuk Awaiting requested review from ibuziuk

@yhontyk yhontyk Awaiting requested review from yhontyk

@MichalMaler MichalMaler Awaiting requested review from MichalMaler

Assignees
No one assigned
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
7.29
Development

Successfully merging this pull request may close these issues.
8 participants
@aditijadhav38 @che-bot @sleshchenko @nickboldt @benoitf @skabashnyuk @themr0c @sr229