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.

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

workspace namespace/project placeholders #14524

Merged
merged 5 commits into from
Sep 19, 2019
Merged

workspace namespace/project placeholders #14524

merged 5 commits into from
Sep 19, 2019

Conversation

sparkoo
Copy link
Member

@sparkoo sparkoo commented Sep 12, 2019

What does this PR do?

Adds possibility to use placeholders in namespace for workspace with current-user values. There are currently 2 placeholders <username> and <userid>.

<something> pattern is not definitive. Please comment in case of any concerns or better ideas.

What issues does this PR fix or reference?

#13488

Docs PR

no docs describing installation and workspace strategies exists yet.

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Sep 12, 2019
@sparkoo sparkoo changed the title namespace for workspace placeholders 🚧 namespace for workspace placeholders Sep 12, 2019
@sparkoo sparkoo changed the title 🚧 namespace for workspace placeholders WIP namespace for workspace placeholders Sep 12, 2019
@sparkoo sparkoo changed the title WIP namespace for workspace placeholders 🚧 namespace for workspace placeholders Sep 12, 2019
@che-bot
Copy link
Contributor

che-bot commented Sep 12, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 12, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@sparkoo
Copy link
Member Author

sparkoo commented Sep 13, 2019

Tested with oauth on Minishift OS3.11. Creates namespace/project fine.

@che-bot
Copy link
Contributor

che-bot commented Sep 13, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14524

@sparkoo
Copy link
Member Author

sparkoo commented Sep 13, 2019

ci-build

@sparkoo sparkoo marked this pull request as ready for review September 13, 2019 09:10
@sparkoo
Copy link
Member Author

sparkoo commented Sep 13, 2019

Q:

  • what to do when che is not using OS oauth and ServiceAccount can't create new project? Should we handle it in code? Or let it fail and just document how it must be configured to work?
  • how we should support it on k8s? Current PR handles also that, but it probably won't work due to lack of permissions. Should we ignore it on k8s at the moment?

@che-bot
Copy link
Contributor

che-bot commented Sep 13, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@sleshchenko
Copy link
Member

  • what to do when che is not using OS oauth and ServiceAccount can't create new project? Should we handle it in code? Or let it fail and just document how it must be configured to work?

So, these placeholders may work in such cases:

  • OpenShift Oauth is configured;
  • Service account is configured with admin cluster role; <- I'm not sure that it's production use case at all, so maybe for simplification, we should go with: user's related placeholders are supported only when OpenShift Oauth is configured. If yes - then we would be able to report configuration issue earlier: in chectl or che-operator. And of course, it could be handled in the scope of another issue.
  • how we should support it on k8s? Current PR handles also that, but it probably won't work due to lack of permissions. Should we ignore it on k8s at the moment?

There is an issue (or at least there were discussions) to remove cluster-admin permissions for k8s installation, but currently, Che has such permissions by default https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/cluster-role-binding.yaml

@sparkoo
Copy link
Member Author

sparkoo commented Sep 13, 2019

works fine on minikube. As @sleshchenko mentioned, it will stop working when we remove cluster-admin permission.

@sparkoo sparkoo changed the title 🚧 namespace for workspace placeholders workspace namespace/project placeholders Sep 16, 2019
@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@dmytro-ndp
Copy link
Contributor

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14524

Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

in general looks good

this.serviceAccountName = serviceAccountName;
this.clusterRoleName = clusterRoleName;
this.clientFactory = clientFactory;
}

private boolean hasPlaceholders(String namespaceName) {
for (String placeholder : NAMESPACE_NAME_PLACEHOLDERS.keySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt may be stream-based implementation would look clener?

Copy link
Member Author

Choose a reason for hiding this comment

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

might be, I don't have strong preference here. I've changed it to stream, but kept it in own method.

@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14524

@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@sparkoo
Copy link
Member Author

sparkoo commented Sep 16, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Sep 16, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@sparkoo
Copy link
Member Author

sparkoo commented Sep 17, 2019

ci-test

@SkorikSergey
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/2316//Selenium_20tests_20report/) doesn't show any regression against this Pull request.

@che-bot
Copy link
Contributor

che-bot commented Sep 17, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

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

@che-bot
Copy link
Contributor

che-bot commented Sep 18, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14524

@che-bot
Copy link
Contributor

che-bot commented Sep 19, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@sparkoo
Copy link
Member Author

sparkoo commented Sep 19, 2019

ci-build

@sparkoo sparkoo merged commit 6d9cfe6 into eclipse-che:master Sep 19, 2019
@sparkoo sparkoo deleted the namespacePerUser branch September 19, 2019 23:02
@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 Sep 19, 2019
@che-bot che-bot added this to the 7.2.0 milestone Sep 19, 2019
@sparkoo
Copy link
Member Author

sparkoo commented Sep 19, 2019

I've tested with cluster-admin role for che serviceaccount. Works fine, same as on k8s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants