Everyone contributing to this PR have now signed the CLA. Thanks!

Hi @skatsaounis,

Thank you for the thorough review! It's my first time working directly with Terraform providers so the feedback is very useful to me. I have applied most of the requested changes already, but I have an issue with setting the database ID as the Terraform ID instead of the name. The reason for this is that the gomaasclient will only accept names as arguments to Get, Update and Destroy. If I try using the numeric database ID, I get 404 errors. I wrote this quick snippet to show you what I get.

// client_tests.go
package main

import (
        "fmt"
        "os"
        "terraform-provider-maas/maas"
)

func playground() error {
        config := maas.Config{
                APIURL: os.Getenv("MAAS_API_URL"),
                APIKey: os.Getenv("MAAS_API_KEY"),
                ApiVersion: "2.0",
        }

        client, err := config.Client()
        if err != nil {
                return err
        }

        zoneByName, errByName := client.Zone.Get("az1")
        zoneByID, errByID := client.Zone.Get("3")
        fmt.Printf("zoneByName: %+v\n", zoneByName)
        fmt.Printf("errByName: %+v\n", errByName)
        fmt.Printf("zoneByID: %+v\n", zoneByID)
        fmt.Printf("errByID: %+v\n", errByID)

        return nil
}

func main() {
        err := playground()
        fmt.Println(err)
}

$ go run client_playground.go
zoneByName: &{Name:az1 Description: ResourceURI:/MAAS/api/2.0/zones/az1/ ID:3}
errByName: <nil>
zoneByID: &{Name: Description: ResourceURI: ID:0}
errByID: ServerError: 404 Not Found (No Zone matches the given query.)

For clarity, the ID and name do match as can be seen in zoneByName.

If we want to use the database ID as the Terraform ID, I think we would need to rely on findZone to get the zone entity, then using zone.Name as the argument for the client, but this solution would add MAAS API calls any time we need to find the zone name from the database ID. For example:

params := getZoneParams(d)
zone, err := getZone(d.Id())
if err != nil {
        return diag.FromErr(err)
}

zone, err = client.Zone.Update(zone.Name, params)
if err != nil {
        return diag.FromErr(err)
}

Please advise on how to proceed with name vs ID. Is there a better approach?

Hi @peterctl ,

The fact that MAAS API v2 is using the zone name instead of the database ID is problematic by itself. Since we cannot change the contract, it is what it is. With the v3 MAAS API we aim to solve this issue among others, but it is not there yet. As you spotted, the findZone is what you need here. We may add an extra API call, but it is a cheap one since zones response is small and we should expect something in the magnitude of tens or hundreds. What is the benefit though? Imagine that someone outside terraform, updated the zone name but there are already existing machines assigned to that zone. What you would expect from Terraform is to identify that change and try to update the same zone, instead of creating a new record. One of the weird behaviors of the API v2 is that you can use /zones/zone_name/update?name=new_zone_name, but again it is what it is.

Please proceed with the database ID as the Terraform resource ID.

In addition, we have to add a field of machines, like we do with tags so that we can assign the zone to machines with Terraform. I forgot to tell you in the first review. An alternative would be a new resource maas_machine_zone_assignment or maas_zone_machine_assignment or another alternative. I am undecided about which one is the best approach. But we can start with the machines field like tags to introduce a similar mechanism.