-
Notifications
You must be signed in to change notification settings - Fork 64
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
fix: Check amount users when OAuth is enabled. #683
Conversation
Signed-off-by: Oleksandr Andriienko <[email protected]>
@AndrienkoAleksandr |
Hello, we can use flag --os-oauth:
|
src/tasks/platforms/crc.ts
Outdated
@@ -54,6 +57,16 @@ export class CRCHelper { | |||
}, | |||
VersionHelper.getOpenShiftCheckVersionTask(flags), | |||
VersionHelper.getK8sCheckVersionTask(flags), | |||
{ | |||
title: 'Verify amount "crc" users', |
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.
Do we really care about users number?
Would it better to just Verify the existence of users?
src/tasks/platforms/crc.ts
Outdated
enabled: () => flags['os-oauth'], | ||
task: async (_ctx: any, task: any) => { | ||
if (await kube.getAmoutUsers() === 0) { | ||
command.error(`No real user exists in the "crc" cluster. Either disable OpenShift OAuth integration("os-oauth" flag) or add at least one user (details in the Help link): "${HOW_TO_CREATE_USER_OS4}"`) |
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 really understand why we have such a requirement for OpenShift Cluster to have a real user...
I could imagine a new OS cluster where admin set up everything before anybody use it:
- add OAuth, like htpasswd;
- install Che (and now it's not possible, because admin except setting up OAuth on cluster must ask anybody or register with set up OAuth on the cluster)...
Why we can't just tell a warning after Che is installed - Hey, you installed Che but it's not possible to use kube:admin on the Che. Please use some other logging methods
?
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.
@AndrienkoAleksandr @sleshchenko we recently had some feedback about that. The use case for a cluster with only kubeadmin is pretty common. That is something we need to support better.
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.
Why we can't just tell a warning after Che is installed
Sorry, but Che won't be installed. Installation will fail. We should notify user as fast, as we can about an issues, that's why we are using pre-flight checks. Otherwise user will wait 5-10 minutes to get broken installation.
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 know about that check on the Operator side but could you elaborate more why Che can't be installed without a real user?
Signed-off-by: Oleksandr Andriienko <[email protected]>
Closed on favour #692 |
What does this PR do?
Check amount users when OAuth is enabled.
What issues does this PR fix or reference?
eclipse-che/che#16629
Signed-off-by: Oleksandr Andriienko [email protected]