Add static validation to middleware #106
Conversation
katherinelc321
requested review from
petrkotas,
s-amann,
mbarnes,
AldoFusterTurpin,
bennerv and
mjlshen
as code owners
|
|
||
| if resourceName != "" { | ||
| if !rxResourceGroupName.MatchString(resourceName) { | ||
| arm.WriteError(w, http.StatusBadRequest, arm.CloudErrorCodeResourceNotFound, "", "The Resource '%s/%s/%s' under resource group '%s' was not found.", resourceProviderNamespace, resourceType, resourceName, resourceGroupName) |
There was a problem hiding this comment.
This doesn't actually check if the resourceName exists within a resourceGroup. If we are trying to validate the resourceName, we should have an error message tailored to the check. Do the resourceGroup and resourceName have the same requirements?
There was a problem hiding this comment.
Each resource has their own requirements. The resource group one is a good upper limit. I can also narrow it down to this from the api spec:
"name": "hcpOpenShiftClusterName",
"in": "path",
"description": "Name of HCP cluster",
"required": true,
"type": "string",
"minLength": 3,
"maxLength": 24,
"pattern": "^[a-zA-Z0-9-]{3,24}$"
}
|
Please rebase pull request. |
katherinelc321
requested review from
UlrichSchlueter and
jharrington22
as code owners
|
Please rebase pull request. |
There was a problem hiding this comment.
Hi @katherinelc321 thank you for doing the extra validation. I agree it is important to do it right from the start. Overall the PR is on a good path with some minor tweaks needed. Would you please look at the comments ?
What this PR does
Before this PR:
Not enough static validation on the frontend
After this PR:
Added validation for subscription ID, resource name, resource group name
Jira: ARO-5736
Link to demo recording:
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.