New Resource: vault_ldap_auth_backend #126
Conversation
|
Awesome! Hope it will get merged soon. |
|
@catsby - any chance we can have someone look at this PR && other PRs on this repo? Lots of pending PRs and no new releases since early April. My client is eager to have their Vault Enterprise configuration reflected in code, and it's more user friendly to manage this with the Vault provider than via bash scripts. @paddycarver @bflad sorry to spam, but hoping we can get someone to look at this. |
|
@angrylogic - after pushing this and testing, have you encountered any issues with the LDAP backend? I'm going to test it soon, as I'm a frustrated with the lack of attention to this PR, but I figured I would ask you if I should expect any issues before testing it out myself. Let me know at your earliest convenience. Thanks for your work!! |
vault/resource_ldap_auth_backend.go
Outdated
| Exists: ldapAuthBackendExists, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "url": &schema.Schema{ |
There was a problem hiding this comment.
Would be nice to get all &schema.Schema declarations like this stripped out.
| _, err := client.Logical().Write(path, data) | ||
|
|
||
| if err != nil { | ||
| d.SetId("") |
There was a problem hiding this comment.
Does this also need to be called above elsewhere to set it to a non-empty-string value?
There was a problem hiding this comment.
I'm not sure if this is the "correct" way of handling this. The idea here is that the ID gets set initially inside the Create function (ldapAuthBackendWrite). This is the Update function (ldapAuthBackendUpdate) where the ID should already have been set.
This code basically says "if we failed to update this resource inside Terraform, we should remove it from the state entirely because its probably not there any longer".
There were some other resources with this behaviour at the same time which I copied, but I'm not sure its the correct way to handle an error here.
| case json.Number: | ||
| apiData, err := resp.Data[apiAttr].(json.Number).Int64() | ||
| if err != nil { | ||
| return fmt.Errorf("Expected API field %s to be an int, was %q", apiAttr, resp.Data[apiAttr]) |
There was a problem hiding this comment.
Would be nice to have all errs be lowercase.
| func arrayToTerraformList(values []string) string { | ||
| output := make([]string, len(values)) | ||
| for idx, value := range values { | ||
| output[idx] = fmt.Sprintf(`"%s"`, value) |
There was a problem hiding this comment.
Is this step needed? I'm wondering if it would be possible to just do this:
return fmt.Sprintf("[%s]", strings.Join(values, ", "))
And literally delete everything else inside this function.
There was a problem hiding this comment.
Yes, this is a much better way to do this. My go was rusty at the time I wrote these :-)
EDIT: Actually - the problem here is we need to quote these values to make them an array of strings for Terraform which is what the intermediate step is doing.
There was a problem hiding this comment.
Oh yes, that makes sense! I didn't catch the interior quotes when looking the first time.
This also includes `vault_ldap_auth_backend_user` and `vault_ldap_auth_backend_group`. If you prefer I can split these out into separate PRs however it seemed to make sense to include them all at once since they are not useful without their counterparts.
* lowercase errors * remove unneeded &schema.Schema declarations
angrylogic
force-pushed
the
resource_ldap_auth_backend
branch
from
1a49383
to
0446eea
Compare
ghost
added
the
…ackend New Resource: vault_ldap_auth_backend
This also includes
vault_ldap_auth_backend_userandvault_ldap_auth_backend_group. If you prefer I can split these outinto separate PRs however it seemed to make sense to include them all at
once since they are not useful without their counterparts.