[Enhancement] Improve error messages for API configuration save

[gbraad]

When a value fails validation, the error message is returned in the body. When multiple errors occur, they are concatenated in the body. This makes parsing very difficult in the applications using this:

Getting:
500

  body: "Value '6144' for configuration property 'memory' is invalid, reason: requires memory in MiB >= 9216\n" +
    "Value '19' for configuration property 'disk-size' is invalid, reason: requires disk size in GiB >= 31\n"

Is not the same as:
500

[
   {
      "value":"memory",
      "default":"9216",
      "request":"6144",
      "reason":"invalid",
      "requires":">= 9216"
   },
   {
      "value":"disk-size",
      "default":"31",
      "request":"19",
      "reason":"invalid",
      "requires":">= 31"
   }
]

[gbraad]

Related to:

• [Task] Add a way to distinguish if a value is default or what the default value is #2869
• Notification does not handle error when value below minimum/default tray-electron#120

[cfergeau]

This can be reproduced by adding this to the test suite:

diff --git a/pkg/crc/api/api_http_test.go b/pkg/crc/api/api_http_test.go
index 2d743126..b0db23a2 100644
--- a/pkg/crc/api/api_http_test.go
+++ b/pkg/crc/api/api_http_test.go
@@ -369,6 +369,16 @@ var testCases = []testCase{
                request:  get("config?cpus"),
                response: jSon(`{"Success":true,"Error":"","Configs":{"cpus":4}}`),
        },
+       {
+               request:  post("config?cpus").withBody(`{"properties":{"cpus":4}}`),
+               response: httpError(200).withBody(`{"Success":true,"Error":"","Properties":["cpus"]}`),
+       },
+       {
+               request: post("config?cpus").withBody(`{"properties":{"cpus":0,"memory":0}}`),
+               response: httpError(500).withBody(`Value '0' for configuration property 'cpus' is invalid, reason: requires CPUs >= 4
+Value '0' for configuration property 'memory' is invalid, reason: requires memory in MiB >= 9216
+`),
+       },
 }
 
 var invalidHTTPMethods = []testCase{

[cfergeau]

I'd suggest improving the error types internally in crc so that we can serialize them to json in a more meaningful way. Rather than returning strings from validation functions, we could return more complex error types, which could be serialized to json in a more useful way.
Quick PoC at cfergeau@15639be but this does not even build. Quite a few functions to change/error types to add if we go this way.

[gbraad]

It might also help to describe the defaults in a better way from /api/config, as at the moment they had to be defined in the Configuration component of the client.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[gbraad]

@anjannath has this been resolved?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[evidolob]

@gbraad I think we could extend this issue to all API errors

For example now:
https://github.com/code-ready/crc/blob/0bad8c24d6d3e560ad530913f635b765d5ae97d9/pkg/crc/api/client/client.go#L188-L202

Just ignore any body text/json in error response.

So caller of sendGetRequest just receive error with Error occurred sending GET request to {API uri}: {statusCode}

So hawing some sort of unified way to send/handle errors from daemon API is a good enhancement