-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Surfaces errors from namespace delete properly #19483
Conversation
4cb1dbc
to
04e106e
Compare
Note: I still need to test this manually. I will update this comment once I do. EDIT: Have now manually tested and confirmed that the error is bubbling up to the API response - see below. |
04e106e
to
fb7de68
Compare
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.
LGTM with wording suggestion on the changelog.
If we really wanted to we could also add a test to ensure non-default namespace deletions return an error; but I assume this is also caught by state store testing.
@@ -0,0 +1,3 @@ | |||
```release-note:bug | |||
namespaces: Failed delete calls no longer return success codes |
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.
namespaces: Failed delete calls no longer return success codes | |
namespaces: Fixed a bug where a 200 OK response was returned even when the namespace delete failed |
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.
LGTM!
Adds a "return err" that was accidentally dropped shortly after the Namespace OSS migration.
fixes #19414