Addition of resource and data source for tenant_policies_igmp_interface_policy (DCNE-278) #403

shrsr · 2025-11-27T18:11:01Z

No description provided.

akinross · 2025-11-28T14:14:10Z

mso/utils.go

 	return nil
 }

+func removePatchPayloadToContainer(payloadContainer *container.Container, op, path string) error {


why do you need a new function for this? cant you pass in nil to value in the addPatchPayloadToContainer function and check if value nil then change the payload to not include value?

I actually think it's better to have a dedicated function for remove. If we want to have both add and remove functionality in the same function it would be best to update the name of addPatchPayloadToContainer to updatePatchPayloadToContainer or similar.

I think the naming of add is not linked to the patch operation but the adding of a PATCH payload to the container. So whatever operation add/replace/remove the PATCH payload has could be handled by same function. If you think it is clearer this way should we then also add separate function for replace?

True. It's not operation related..

akinross · 2025-11-28T14:17:27Z

mso/resource_mso_tenant_policies_igmp_interface_policy.go

+	if d.HasChange("state_limit_route_map_uuid") {
+		uuid := d.Get("state_limit_route_map_uuid").(string)
+		if uuid != "" {
+			err := addPatchPayloadToContainer(payloadCont, "replace", fmt.Sprintf("%s/stateLimitRouteMapRef", updatePath), uuid)
+			if err != nil {
+				return err
+			}
+		} else {
+			err := removePatchPayloadToContainer(payloadCont, "remove", fmt.Sprintf("%s/stateLimitRouteMapRef", updatePath))
+			if err != nil {
+				return err
+			}
+		}
+	}


does this work properly in older sdk version? i thought there was not a clear way to distinguish between not provided and empty string?

Yes, it works well.
If not provided the ok in ok, val:=d.GetOk("some_string") won't have true and d.HasChange("some_string") won't be true either.

Well it works that a change is detected on "" or commented out. However both delete the configuration, because we cannot differentiate between empty string or not provided configuration. So let me rephrase my question is it the intend that commenting out the config or providing null explicitly also leads to destroy operation to be invoked?

yes, both removing/commenting the attribute and its value or providing "" will invoke the remove operation. I actually removed the redundant code in the latest change.

akinross · 2025-11-28T14:17:40Z

mso/resource_mso_tenant_policies_igmp_interface_policy.go

+			}
+		}
+	}
+	log.Printf("HERE %v", payloadCont)


akinross · 2025-12-04T09:45:33Z

examples/tenant_policies_igmp_interface_policy/main.tf

+}
+
+resource "mso_tenant_policies_route_map_policy_multicast" "state_limit" {
+  template_id = mso_template.template_tenant.id


Suggested change

template_id = mso_template.template_tenant.id

template_id = mso_template.tenant_template.id

akinross · 2025-12-04T09:54:30Z

examples/tenant_policies_igmp_interface_policy/main.tf

+# tenant policies igmp interface policy example
+
+resource "mso_tenant_policies_igmp_interface_policy" "igmp_policy" {
+  template_id                  = mso_template.template_tenant.id


Suggested change

template_id = mso_template.template_tenant.id

template_id = mso_template.tenant_template.id

samiib

LGTM

akinross

LGTM

gmicol

LGTM

…es_igmp_interface_policy

…s_igmp_interface_policy

…ons used in resource_mso_tenant_policies_igmp_interface_policy

…ies_igmp_interface_policy

…enant_policies_igmp_interface_policy

…_interface_policy

akinross · 2025-12-18T10:26:47Z

mso/resource_mso_tenant_policies_igmp_interface_policy.go

+		return err
+	}
+
+	d.SetId(fmt.Sprintf("templateId/%s/IGMPInterfacePolicy/%s", templateId, d.Get("name").(string)))


Is there a reason to capitalise IGMP on the setID when the list equivalent is igmpInterfacePolicies? What is the rule we follow for setting this?

I don't remember why I went with capitalizing for the first resource (IPSLA) I created but this is the rule we've been following for all the the NDO TF Ids

akinross · 2025-12-18T10:28:59Z

mso/resource_mso_tenant_policies_igmp_interface_policy.go

+		d.Set("report_link_local_groups", reportLinkLocal)
+	}
+
+	d.Set("igmp_version", models.StripQuotes(response.S("igmpQuerierVersion").String()))


why is there no response.Exists() for all of the attributes to be set? is this because some would always exist?

I missed it here

akinross · 2025-12-18T10:33:04Z

mso/resource_mso_tenant_policies_igmp_interface_policy.go

+		enableV3Asm, _ := response.S("enableV3Asm").Data().(bool)
+		d.Set("version3_asm", enableV3Asm)


is bool casting not working directly?, what is the _ for, an error? Is there a case it can error?

the _ should actually be 'ok' indicating whether the type assertion succeeded. However, we're using response.Exists() so it should be fine leaving it blank. I don't think we need it the assertion check because API always returns the same type unless they were to change true to enable at some point. I modified to include ok here.

…nterface_policy to make type assertion checks

akinross

LGTM

shrsr self-assigned this Nov 27, 2025

shrsr requested review from akinross, anvitha-jain, gmicol, lhercot, sajagana and samiib as code owners November 27, 2025 18:11

shrsr linked an issue Nov 27, 2025 that may be closed by this pull request

Create a resource and data source for "IGMP Interface Policy" object under Tenant Policies (DCNE-278) #325

Open

github-actions bot changed the title ~~Addition of resource and data source for tenant_policies_igmp_interface_policy~~ Addition of resource and data source for tenant_policies_igmp_interface_policy (DCNE-278) Nov 27, 2025

akinross requested changes Nov 28, 2025

View reviewed changes

shrsr requested a review from akinross December 3, 2025 17:57

akinross requested changes Dec 4, 2025

View reviewed changes

shrsr requested a review from akinross December 4, 2025 16:39

samiib previously approved these changes Dec 5, 2025

View reviewed changes

akinross previously approved these changes Dec 10, 2025

View reviewed changes

gmicol previously approved these changes Dec 10, 2025

View reviewed changes

shrsr added 5 commits December 15, 2025 13:44

[minor_change] Addition of resource and data source for tenant_polici…

9453aca

…es_igmp_interface_policy

[ignore] Removed debug log statement from resource_mso_tenant_policie…

a07e66a

…s_igmp_interface_policy

[ignore] Modified addPatchPayloadToContainer to handle remove operati…

bb22bf6

…ons used in resource_mso_tenant_policies_igmp_interface_policy

[ignore] Removed redundant code for remove operations in tenant_polic…

653afb7

…ies_igmp_interface_policy

[ignore] Reverted code to invoke remove op on certain conditions in t…

0d2496a

…enant_policies_igmp_interface_policy

shrsr dismissed stale reviews from gmicol, akinross, and samiib via 0d2496a December 15, 2025 18:47

shrsr force-pushed the igmp branch from cf54b07 to 0d2496a Compare December 15, 2025 18:47

[ignore] Removal of default values from docs for tenant_policies_igmp…

d843f09

…_interface_policy

shrsr requested review from akinross, gmicol and samiib December 16, 2025 03:29

akinross requested changes Dec 18, 2025

View reviewed changes

[ignore] Modified setting data in resource_mso_tenant_policies_igmp_i…

880d03f

…nterface_policy to make type assertion checks

shrsr requested a review from akinross December 18, 2025 16:05

akinross approved these changes Dec 18, 2025

View reviewed changes

	template_id = mso_template.template_tenant.id
	template_id = mso_template.tenant_template.id

		enableV3Asm, _ := response.S("enableV3Asm").Data().(bool)
		d.Set("version3_asm", enableV3Asm)

Addition of resource and data source for tenant_policies_igmp_interface_policy (DCNE-278) #403

Are you sure you want to change the base?

Addition of resource and data source for tenant_policies_igmp_interface_policy (DCNE-278) #403

Uh oh!

Conversation

shrsr commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samiib left a comment

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

gmicol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shrsr Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akinross Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shrsr Dec 18, 2025 •

edited

Loading

akinross Dec 18, 2025 •

edited

Loading