refactor: Throwing error messages with field. #3239
Conversation
|
Build Failed 😱 Build Id: f1435361-dd62-4200-bc19-27ccc3842d23 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
aimuz
changed the title
google-oss-prow
bot
added
do-not-merge/work-in-progress
size/XL
and removed
size/L
labels
|
Build Failed 😱 Build Id: d2a43874-b71c-4bdf-8167-088e1e325644 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
ref #3103 Related to this, in order to describe more information in Message without having to perform type conversion |
|
Build Failed 😱 Build Id: fe92ab53-a0aa-4693-a268-2dcef8f29cab To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 1735c2d3-1c58-44ca-8bd9-216e9edff77f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Make sure you are using the correct google account when viewing the page. |
|
Thanks for all this work, but what's not clear to me is why this is an improvement? What does this actually do? Is this a consolidation of code? A pure refactor? An improvement on user experience? Something else? If you could write a few sentences on the goal etc of what the aims are that would be super useful, and also aid in reviewing it as well. Does it link back to an existing issue? Whatever resources you can point to would also help. |
There are several purposes for using the same validation as k8s. It allows for reuse of existing code and reduces development time. The error messages thrown by this implementation are more detailed compared to the current one. For example, when creating a fleet with incorrect parameters, the current message is simply "Fleet configuration is invalid". This hides critical information that could be useful for debugging. To address this issue, the proposed changes include implementing the By adopting this approach, it becomes easier to retrieve detailed error information through Purpose of the PRThe motivation behind this pull request is to integrate Agones into a PaaS platform. However, during the integration process, there were errors occurring without clear descriptions, making it difficult to understand the root cause. Currently, the program checks if the error is empty and then directly outputs the result of Through this PR, the aim is to make error handling more standardized and improve the clarity of error messages. The proposed changes in For example, the introduction of this PR enables the following error message: {
"kind": "Status",
"apiVersion": "v1",
"metadata": {},
"status": "Failure",
"message": "Fleet.agones.dev \"AAsimple-game-server\" is invalid: metadata.name: Invalid value: \"AAsimple-game-server\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')",
"reason": "Invalid",
"details": {
"name": "AAsimple-game-server",
"group": "agones.dev",
"kind": "Fleet",
"causes": [
{
"reason": "FieldValueInvalid",
"message": "Invalid value: \"AAsimple-game-server\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')",
"field": "metadata.name"
}
]
},
"code": 422
}In this example, the error message clearly states that the Similarly, when the {
"kind": "Status",
"apiVersion": "v1",
"metadata": {},
"status": "Failure",
"message": "admission webhook \"validations.agones.dev\" denied the request: Fleet configuration is invalid",
"reason": "Invalid",
"details": {
"name": "simple-game-server",
"group": "agones.dev",
"kind": "Fleet",
"causes": [
{
"reason": "FieldValueNotSupported",
"message": "Value cannot be set unless feature flag FleetAllocationOverflow is enabled",
"field": "allocationOverflow"
}
]
},
"code": 400
}As you can see, the error messages generated by kube-apiserver provide more detailed information. Although additional details are available in the The proposed changes aim to make errors more explicit and easier to understand. |
This one can see the log 
This can't see the logs 
|
|
@markmandel Do you have time to help me with the question of why I can't see the logs? |
|
Just to be 100% clear on the logs -- you can see the logs linked from this PR, but I'm guessing not the ones that are the e2e for each version of a gke cluster? (sub-builds)? |
Thank you very much for the detailed description -- this makes things much more clear, and this is sounding like a really good change.
Just because my memory is no good 😄 what sort of output did you see before this change? Also, does this change improve things when kubectl fails? Since this is a bigger change @zmerlynn - would you care to also pass your eyes over this, just for review? |
pkg/apis/agones/v1/common.go
Outdated
| return causes | ||
| func validateObjectMeta(objMeta *metav1.ObjectMeta, fldPath *field.Path) field.ErrorList { | ||
| var allErrs field.ErrorList | ||
| allErrs = append(allErrs, metav1validation.ValidateLabels(objMeta.Labels, fldPath.Child("labels"))...) |
There was a problem hiding this comment.
Looking at this now, what you propose makes lots of sense. Where before we would take the errors and convert them back into structs -- when we never had to do that.
Nice find.
pkg/apis/agones/v1/gameserver.go
Outdated
| if productCauses := apiHooks.ValidateGameServerSpec(gss); len(productCauses) > 0 { | ||
| causes = append(causes, productCauses...) | ||
| for _, cause := range apiHooks.ValidateGameServerSpec(gss) { | ||
| // TODO: Use field.ErrorList |
There was a problem hiding this comment.
Curious - any reason this is a TODO without being implemented yet? Or were you waiting on some review before implementing this?
There was a problem hiding this comment.
APIHOOK is an interface, and I plan to modify the implementation of the interface in the next phase to avoid making too many changes in this PR.
If you think you can do it together in this PR, I will do the work together.
There was a problem hiding this comment.
Got it - that's fine - just wanted to check 👍🏻
There was a problem hiding this comment.
Just a workflow nit - I don't see it much in this project, but consider using something like TODO(#xyz) where xyz is an issue number. Then you can search for it easily to ensure you've accounted for all the TODOs when you're working on a long arc of PRs.
|
Build Failed 😱 Build Id: 67f161aa-2d1e-451f-b4c8-0a51b8676e0a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Looking through the e2e tests (while you can't see them), here are some errors: That should keep you going 😄 |
|
#3239 should fix the access issue for future builds once merged. Thanks for highlighting the issue 👍🏻 |
Thanks, I will continue to complete this work |
Path and field.Error will make the error message update accurately Signed-off-by: aimuz <[email protected]>
Signed-off-by: aimuz <[email protected]>
| if productCauses := apiHooks.ValidateGameServerSpec(gss); len(productCauses) > 0 { | ||
| causes = append(causes, productCauses...) | ||
| for _, cause := range apiHooks.ValidateGameServerSpec(gss) { | ||
| // TODO(#3239): Use field.ErrorList |
|
Build Succeeded 👏 Build Id: bc3cd867-baa7-4892-b928-837d9fee4280 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@markmandel I'm sorry I ignored this message This is the content before modification {
"kind": "Status",
"apiVersion": "v1",
"metadata": {},
"status": "Failure",
"message": "admission webhook \"validations.agones.dev\" denied the request: GameServer configuration is invalid",
"reason": "Invalid",
"details": {
"name": "simple-game-server-ghnf7",
"group": "agones.dev",
"kind": "GameServer",
"causes": [
{
"reason": "FieldValueInvalid",
"message": "ContainerPort cannot be specified with Passthrough PortPolicy",
"field": "default.containerPort"
}
]
},
"code": 400
}Changes to this PR {
"kind": "Status",
"apiVersion": "v1",
"metadata": {},
"status": "Failure",
"message": "admission webhook \"validations.agones.dev\" denied the request: GameServer.agones.dev \"simple-game-server-x8w4m\" is invalid: spec.ports[0].containerPort: Required value: ContainerPort cannot be specified with Passthrough PortPolicy",
"reason": "Invalid",
"details": {
"name": "simple-game-server-x8w4m",
"group": "agones.dev",
"kind": "GameServer",
"causes": [
{
"reason": "FieldValueRequired",
"message": "Required value: ContainerPort cannot be specified with Passthrough PortPolicy",
"field": "spec.ports[0].containerPort"
}
]
},
"code": 422
} |
|
/assign @cyriltovena |
|
/assign @markmandel |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aimuz, markmandel, zmerlynn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
Build Failed 😱 Build Id: 2c297dc0-a716-40a7-bb73-91cbf6a51a61 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Oops, got caught when we were recreating the e2e clusters. Let's hit retry when complete. |
|
Do I need to do something? |
|
Nah, we've got it. |
|
Build Failed 😱 Build Id: bdd94e98-b4ae-4cc1-af4b-d08cd449d932 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
It looks like the timeout is causing the error, do I need to resubmit to trigger the retest? |
|
kicked off another retry |
|
Build Succeeded 👏 Build Id: e4426dfa-2167-4ca0-8e71-2aa1e784d481 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Path and field.Error will make the error message update accurately
Signed-off-by: aimuz [email protected]
What type of PR is this?
/kind breaking
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer:
This PR uses field.ErrorList to record error messages. This will make the errors thrown more standard and easier to locate