fix: add hashing function on region attributes

[MircoT]

With this fix is resolved the #281

[scraly]

Tested and now we have all the regions displayed in regions_attributes field.

Before:

# ovh_cloud_project_network_private.vnet:
resource "ovh_cloud_project_network_private" "vnet" {
    id                 = "pn-1083678_5"
    name               = "test"
    regions            = [
        "GRA7",
        "SBG5",
    ]
    regions_attributes = [
        {
            openstackid = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx"
            region      = "SBG5"
            status      = "ACTIVE"
        },
    ]
    regions_status     = [
        {
            region = "SBG5"
            status = "ACTIVE"
        },
    ]
    service_name       = "xxxxxxxxx"
    status             = "ACTIVE"
    type               = "private"
    vlan_id            = 5
}

After:

# ovh_cloud_project_network_private.vnet:
resource "ovh_cloud_project_network_private" "vnet" {
    id                 = "pn-1083678_5"
    name               = "test"
    regions            = [
        "GRA7",
        "SBG5",
    ]
    regions_attributes = [
        {
            openstackid = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx"
            region      = "GRA7"
            status      = "ACTIVE"
        },
        {
            openstackid = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx"
            region      = "SBG5"
            status      = "ACTIVE"
        },
    ]
    regions_status     = [
        {
            region = "SBG5"
            status = "ACTIVE"
        },
    ]
    service_name       = "xxxxxx"
    status             = "ACTIVE"
    type               = "private"
    vlan_id            = 5
}

[scraly]

Why are you returning 0?
In which case should we obtain ok == false?
Is it possible to return an error message or something?
Thanks

[MircoT]

Actually, I'm returning 0 to be conformed to the hashing function signature, that is func(interface{}) int. Said that, the conversion check regarding the v interface is just to ensure that everything goes well but, in case of any failure, the only returning option for this signature is an integer and not an error. Maybe, a -1 is a better option in place of returning 0.

However, since the schema should not change, that conversion check is just a stretch. Let me know the way is better to modify. Also, having a code like this is an option:

// HashMapRegionAttributes creates an hash for the region attributes.
func HashMapRegionAttributes(v interface{}) int {
	attributes := v.(map[string]interface{})
	builder := strings.Builder{}

	for _, key := range []string{"status", "region", "openstackid"} {
		value, inMap := attributes[key]
		if inMap {
			stringValue, ok := value.(string)
			if ok {
				builder.WriteString(stringValue)
			}
		}
	}

	return schema.HashString(builder.String())
}

[MircoT]

Currently, I opted for that last option. I hope it is the best way to approach the problem.

[scraly]

The error must be handled.

For example, the method can return an int and an error:

// HashMapRegionAttributes creates an hash for the region attributes.
func HashMapRegionAttributes(v interface{}) (int, error) {
    attributes, ok := v.(map[string]interface{})
    if !ok {
        return -1, errors.New("bad cast...")
    }

    builder := strings.Builder{}
...

[MircoT]

@scraly it is not possible to handle the error in that function because in a Terraform Schema the Set field is a SchemaSetFunc that should be this specific type:

type SchemaSetFunc func(interface{}) int

Consequently, returning a tuple instead of an integer is not an option at this level.

[MircoT]

The error must be handled.

For example, the method can return an int and an error:

// HashMapRegionAttributes creates an hash for the region attributes.
func HashMapRegionAttributes(v interface{}) (int, error) {
    attributes, ok := v.(map[string]interface{})
    if !ok {
        return -1, errors.New("bad cast...")
    }

    builder := strings.Builder{}
...

Another option that doesn't involve many code changes in resource creation could be something like this:

"regions_attributes": {
		Type:     schema.TypeList,
		Computed: true,
		Elem: &schema.Resource{
			Schema: map[string]*schema.Schema{
				"status": {
					Type:     schema.TypeString,
					Required: true,
				},

				"region": {
					Type:     schema.TypeString,
					Computed: true,
				},

				"openstackid": {
					Type:     schema.TypeString,
					Computed: true,
				},
			},
		},
	}

With such a solution, the helper function HashMapRegionAttributes for the set is not required and the logic behind the list creation is still the same. Otherwise, if a strict check of the element structures (beyond the schema) is necessary, the TypeMap could be used with a SchemaValidateDiagFunc but, in that case, the resource creation has to be changed too.

Which one is better for this case @scraly ?

PS: of course, in the case of TypeList utilization, I'll remove this PR and make a new one with only that difference

[scraly]

Hi, after thinking,
is it possible to:

• move the function to cloud_project_network_private_helpers.go because the function should. be only used by the resource
• rename the function to RegionAttributesHash for example
• add an error log when the cast is failing and a panic

Thanks

[scraly]

Hi, after thinking,
is it possible to:

• move the function to cloud_project_network_private_helpers.go because the function should. be only used by the resource
• rename the function to RegionAttributesHash for example
• add an error log when the cast is failing and a panic

Thanks

[MircoT]

Hi, after thinking, is it possible to:

• move the function to cloud_project_network_private_helpers.go because the function should. be only used by the resource
• rename the function to RegionAttributesHash for example
• add an error log when the cast is failing and a panic

Thanks

All the requirements should be satisfied, let me know if something else is missing or if the error description is not conforming to the others in the module.

[scraly]

The only thing left to comply with the whole list is to put the function in a file next to the resource, for example: cloud_project_network_private_helpers.go :)

[MircoT]

The only thing left to comply with the whole list is to put the function in a file next to the resource, for example: cloud_project_network_private_helpers.go :)

Right... I just misunderstood the previous message...

[scraly]

Thanks, now it's perfect :)