Implement namespace management. #26
Conversation
Allow Terraform to manage Nomad namespaces. At the moment, we have an unadvertised parameter for setting a quota on the namespace, as well. This will be tested and then advertised in a future commit, after quota management with Terraform is merged.
There was a problem hiding this comment.
hey @paddycarver
I've taken a look through and left some comments in-line - they're mostly minor nits; the one issue which needs to be resolved before merging would be sorting the missing documentation entry
Thanks!
nomad/resource_namespace.go
Outdated
| log.Printf("[DEBUG] Creating namespace %q", namespace.Name) | ||
| _, err := client.Namespaces().Register(&namespace, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("error inserting namespace %q: %s", namespace.Name, err.Error()) |
There was a problem hiding this comment.
minor given the comment above, should this be upserting?
nomad/resource_namespace.go
Outdated
| return resourceNamespaceRead(d, meta) | ||
| } | ||
|
|
||
| func resourceNamespaceUpdate(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
minor: given these methods are the same - we can combine them for now?
nomad/resource_namespace.go
Outdated
| Quota: d.Get("quota").(string), | ||
| } | ||
|
|
||
| // upsert our namespace |
There was a problem hiding this comment.
minor: I think we can remove this, since it's covered by the print statement below?
nomad/resource_namespace.go
Outdated
| client := meta.(*api.Client) | ||
| name := d.Id() | ||
|
|
||
| // delete the namespace |
There was a problem hiding this comment.
minor: I think we can remove this, since it's covered by the print statement below?
nomad/resource_namespace.go
Outdated
| if err != nil { | ||
| // As of Nomad 0.4.1, the API client returns an error for 404 | ||
| // rather than a nil result, so we must check this way. | ||
| if strings.Contains(err.Error(), "404") { |
There was a problem hiding this comment.
I'm assuming that means the status code also isn't a 404?
There was a problem hiding this comment.
It probably is, unfortunately the client library doesn't expose that to me :/
There was a problem hiding this comment.
Please link to hashicorp/nomad#1849 in this comment so we can follow up later more easily.
nomad/resource_namespace.go
Outdated
| } | ||
| if resp == nil { | ||
| // just to be sure | ||
| return false, nil |
There was a problem hiding this comment.
this probably wants a log statement here
There was a problem hiding this comment.
Good catch, thanks!
| The following arguments are supported: | ||
|
|
||
| - `name` `(string: <required>)` - A unique name for the namespace. | ||
| - `description` `(string: "")` - A description of the namespace. |
There was a problem hiding this comment.
I might be wrong, but isn't the convention:
* `name` - (Required) A unique name for the namespace. Changing this forces a new resource to be created.
* `description` - (Optional) Description for this namespace.
* `quota` - (Optional) Quota for this namespace.
(note: quota is missing above)
There was a problem hiding this comment.
I think in general, yes, but when I got to the provider, it already had the above format, so I was trying to keep it consistent within the provider :/ Happy to do a sweep to update, though I have to admit I do think the version I copied is a bit more informative, providing information on defaults and types.
There was a problem hiding this comment.
Oh, also, quota is intentionally missing until #25 gets merged, because I have no way to test it until then. I just included the code here because I was already in the code, but I don't want to document it until I can actually test it.
Remove unnecessary log lines, combine update/create methods.
paddycarver
dismissed
tombuildsstuff’s stale review
Made suggested changes, left feedback on documentation feedback.
nomad/resource_namespace.go
Outdated
| if err != nil { | ||
| // As of Nomad 0.4.1, the API client returns an error for 404 | ||
| // rather than a nil result, so we must check this way. | ||
| if strings.Contains(err.Error(), "404") { |
There was a problem hiding this comment.
Please link to hashicorp/nomad#1849 in this comment so we can follow up later more easily.
Allow Terraform to manage Nomad namespaces. At the moment, we have an
unadvertised parameter for setting a quota on the namespace, as well.
This will be tested and then advertised in a future commit, after quota
management with Terraform (#25) is merged.