Enhanced API to catch Settings validation errors

[abellotti]

Enhanced API to catch Settings validation errors and reporting accordingly to the client via a BadRequestError.

[abellotti]

Marking as WIP for now, tests will fail until we update/sync up with @Fryguy's magic settings fix.

[imtayadeway]

The other server settings specs are in spec/requests/servers_spec.rb - would it make sense for this to live there too?

[abellotti]

starting going that way, but then realizing I'm really testing a settings update handling that's generic to all and it didn't make sense to duplicate for servers, zone, region, so I put that here.

[imtayadeway]

Right, but you're hitting a different endpoint to the one suggested by this file. Does it work if you hit the api_settings_url? If not, perhaps consider a shared example to eliminate duplication?

[abellotti]

ahh good point, I'll move it the proper file.

[imtayadeway]

I'm not opposed to extracting a method here, but I think its name should reflect the responsibilit(y|ies) of the new method - here's it's to try an update and raise on a validation error. I'm having a hard time trying to find a succinct way to say that, so maybe inline for now?

[abellotti]

wasn't crazy about have the begin/rescue inside the when block, (you never know when rubocop starts complaining about complexity 😉 ) so I created the separate method. I can go either way really. up to you.

[abellotti]

poke @imtayadeway any preference ?

[imtayadeway]

I think my preference is for in-line 👍

[miq-bot]

Checked commits abellotti/manageiq-api@f21df07~...4f64cec with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[abellotti]

@Fryguy PR updated following ManageIQ/manageiq#17247 Thanks!!

[abellotti]

PATCH /api/servers/1/settings

{
  "authentication" : { "bind_timeout" : 123 }
}

now gives us:

{
  "error": {
    "kind": "bad_request",
    "message": "Settings validation failed - authentication-mode: authentication mode, nil, is invalid. Should be one of: amazon, ldap, ldaps, httpd, database, none",
    "klass": "Api::BadRequestError"
  }
}

[abellotti]

poke @imtayadeway

[imtayadeway]

This is good. I would prefer a spec for zones and regions, since the fact that they have a common pathway is an implementation detail to an integration test, but that's more of a nit. 👍