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

Don't fail in deleteIfManaged when the namespace/project is not managed #15688

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Jan 14, 2020

What does this PR do?

Don't fail in deleteIfManaged when the namespace/project is not managed.
Instead, ignore the request silently.

That seems to be the original intention based on the usage of the method
everywhere else but the test suite 8-|

This bug manifested itself as unnecessary warnings about the inability to delete the namespace/project when a workspace was deleted.

What issues does this PR fix or reference?

#15686

…aged.

Instead, ignore the request silently.

That seems to be the original intention based on the usage of the method
everywhere else but the test suite 8-|

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Copy link
Contributor Author

metlos commented Jan 15, 2020

ci-build

@metlos
Copy link
Contributor Author

metlos commented Jan 15, 2020

ci-test

@metlos
Copy link
Contributor Author

metlos commented Jan 15, 2020

crw-ci-test

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

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.

Few comments a bit outside from your changes. However, kubernetes and OS logic is the same, but code is a bit off.

@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/bug Outline of a bug - must adhere to the bug report template. labels Jan 15, 2020
a namespace.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@centos-ci
Copy link

Can one of the admins verify this patch?

…tempt-to-delete-non-managed-namespace

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Copy link
Contributor Author

metlos commented Jan 16, 2020

Few comments a bit outside from your changes. However, kubernetes and OS logic is the same, but code is a bit off.

I agree to the degree that I already filed an issue a couple of months ago :) - #15250

I think we should not embark on such a potentially big refactoring in a small bugfix like this.

@che-bot

This comment has been minimized.

@che-bot

This comment has been minimized.

@che-bot

This comment has been minimized.

@che-bot
Copy link
Contributor

che-bot commented Jan 16, 2020

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

@metlos
Copy link
Contributor Author

metlos commented Jan 16, 2020

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 16, 2020

✅ E2E Happy path tests succeed 🎉

See Details

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

@metlos metlos merged commit 19b1975 into eclipse-che:master Jan 17, 2020
@che-bot che-bot added this to the 7.8.0 milestone Jan 17, 2020
@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 Jan 17, 2020
@ibuziuk ibuziuk mentioned this pull request Jan 17, 2020
21 tasks
vparfonov pushed a commit that referenced this pull request Jan 17, 2020
…aged (#15688)

Don't fail in `deleteIfManaged` when the namespace/project is not managed.
Instead, ignore the request silently.

That seems to be the original intention based on the usage of the method
everywhere else but the test suite 8-|

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants