Update schema and fix various mappings #7
Conversation
There was a problem hiding this comment.
Thanks for the PR. I'm sorry that I never got around to reviewing your original one.
The changes to the code seem reasonable. I've left a couple of small change requests.
My main concern with this PR is the possibility of breaking changes. I've made a high-level diff of the schema to evaluate that. The rest of this comment is about those schema differences.
Most of the changes are new commands and fields, and also the changes to the type of certificate fields. Those should both be fine.
The pkinit_anonymous command was removed by FreeIPA. I don't think there's much we easily do about that. I doubt it will be a problem for users of this library if we remove the command.
The create_reverse params related to DNS and the md5_fingerprint param related to certs have been removed. What will happen if an old FreeIPA server still sends them to us in a response? Will that cause on error in go-freeipa?
| @@ -49,6 +49,10 @@ func toGoType(ipaType string) string { | |||
| return "string" | |||
| case "Decimal": | |||
| return "float64" | |||
| case "Certificate": | |||
| return "interface{}" | |||
There was a problem hiding this comment.
| return "interface{}" | |
| return "string" |
Certificates had type bytes in the old schema (diff), so the generated fields had type string. We should avoid changing that if possible. Is there a reason why you went with interface{} here?
There was a problem hiding this comment.
Yes, there is a reason. Certificates are actually modeled as maps with a __base64__ subfield, so casting them as strings failed.
| "ipaallowedtoperform_write_keys_group", | ||
| "ipaallowedtoperform_write_keys_host", | ||
| "ipaallowedtoperform_write_keys_hostgroup", | ||
| } |
There was a problem hiding this comment.
I guess you added this workaround since you encountered errors when working with "host" objects. Could you please add a small test which reads (and maybe writes) a host object? That would make it easy to detect similar issues after schema changes in the future.
There was a problem hiding this comment.
Indeed, these fields failed to parse because they're not present when creating a new host and are required by the schema.
|
Hi @tehwalris. Have you seen my answers? |
Yes, thanks. I've opened an issue for the |
No description provided.