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

Remove code that supports namespace strategies other than per-user #19763

Merged
merged 8 commits into from
May 12, 2021

Conversation

skabashnyuk
Copy link
Contributor

@skabashnyuk skabashnyuk commented May 6, 2021

What does this PR do?

Remove code that supports namespace strategies other than per-user
Documentation changes - in progress....

Screenshot/screencast of this PR

Знімок екрана 2021-05-07 о 14 52 38

What issues does this PR fix or reference?

Fixes #19536

How to test this PR?

  1. Deploy che-server image quay.io/skabashn/che-server:che19536
  2. Should create per-user k8s namespace by default
  3. Setup oc patch checluster eclipse-che -n eclipse-che --type=json -p '[{"op": "replace", "path": "/spec/devWorkspace/enable", "value": true}]
  4. Che server startup should fail with message Only 'per user' is allowed.Using the <username> or <userid> placeholder is required in the 'che.infra.kubernetes.namespace.default' parameter. The current value is: '<workspaceid>-che'

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.

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@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/task Internal things, technical debt, and to-do tasks to be performed. labels May 6, 2021
…user

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 6, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

…an per-user

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 6, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented May 6, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 7, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 7, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

…19536

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@skabashnyuk skabashnyuk marked this pull request as ready for review May 7, 2021 13:59
@skabashnyuk skabashnyuk requested a review from sparkoo May 7, 2021 13:59
@che-bot
Copy link
Contributor

che-bot commented May 7, 2021

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

…nto che19536

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 7, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

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.

ACK for the che.properties changes

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 11, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Copy link
Member

@sparkoo sparkoo left a comment

Choose a reason for hiding this comment

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

tested on minikube with multiple namespace configurations and pre-created namespaces, works fine.

Do you plan to update che-operator code to reflect these changes? For example che-operator sets CHE_INFRA_KUBERNETES_NAMESPACE_ALLOW__USER__DEFINED in che configmap.

@sparkoo
Copy link
Member

sparkoo commented May 11, 2021

are we sure about removing allow_user_defined ? I can imagine scenarios where admin pre-create more namespaces for single user.

@skabashnyuk
Copy link
Contributor Author

skabashnyuk commented May 11, 2021

@sparkoo

Do you plan to update che-operator code to reflect these changes? For example che-operator sets CHE_INFRA_KUBERNETES_NAMESPACE_ALLOW__USER__DEFINED in che configmap.

I hope that would be a part of the job that @tolusha will do in eclipse-che/che-operator#818.

are we sure about removing allow_user_defined ?

I think we should be strict: one user -> one k8s namespace.

I can imagine scenarios where admin pre-create more namespaces for single user.

I think that would be a misconfiguration.

@tolusha
Copy link
Contributor

tolusha commented May 11, 2021

@skabashnyuk
I will update operator not to set CHE_INFRA_KUBERNETES_NAMESPACE_ALLOW__USER__DEFINED in che configmap.

@skabashnyuk
Copy link
Contributor Author

docs pr eclipse-che/che-docs#1983

@skabashnyuk skabashnyuk merged commit 14107b5 into eclipse-che:master May 12, 2021
@skabashnyuk skabashnyuk deleted the che19536 branch May 12, 2021 09:45
@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 May 12, 2021
@che-bot che-bot added this to the 7.31 milestone May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the code that support namespace strategies other than "per user"
5 participants