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
KIALI-2034 Fixes for Create Istio Object endpoints #804
Conversation
lucasponce
commented
Jan 24, 2019
- Adjust the right endpoint in the k8s Api
- Parse object to validate type vs payload
- Reformat JSON object used
- Adjust the right endpoint in the k8s Api - Parse object to validate type vs payload - Reformat JSON object used
Tested with:
Or
|
handlers/istio_config.go
Outdated
@@ -304,8 +303,24 @@ func IstioConfigCreate(w http.ResponseWriter, r *http.Request) { | |||
RespondWithError(w, http.StatusBadRequest, "Create request could not be read: "+err.Error()) | |||
} | |||
|
|||
business.IstioConfig.CreateIstioConfigDetail(api, namespace, objectType, objectSubtype, object, string(body)) | |||
json, err := business.IstioConfig.ParseJsonForCreate(objectType, objectSubtype, body) |
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.
Doesn't this kinda ruin the separation? Shouldn't it be a detail of Create?
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 thought that handler layer should process the input parameters and validate those are correct.
Return a BadRequest / 400 error, like a malformed or bad body.
Yeah, this is something in the middle of handlers / business layer.
Return a 404 from the business layer ? and add a IsBadRequest() after the CreateIstioConfigDetail() ?
I can do that if it's more clear.
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 think it would make more sense, given that we return 404 from business layer also later. Thus, the handler should probably only verify the JSON can be extracted and not care about the actual data.
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.
Ok, will move it inside the business and also check the 400 in the returning error, that will place same logic together.
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.
@burmanm I've sent a commit to move this logic, let me know if the PR works for you.
@hhovsepy I don't see why the e2e are failing as there is not a change on them and the logic introduced is new. |
Appears to work, although I would say our "DELETE" answer is incorrect - it's not JSON.
|