Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom extensible attributes for infoblox_ip_allocation resource #101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AliAllomani
Copy link

Re-openning #99

Related to #81

Example

resource "infoblox_ip_allocation" "foo"{
	network_view_name="default"
	vm_name="test-name"
	cidr="10.0.0.0/24"
	ip_addr="10.0.0.1"
	tenant_id="foo"
	extattrs = {
		"Site" = "Site2"
		"IB Discovery Owned" = "Test"
	}
	}

@somashekhar
Copy link
Contributor

When we create an EA we can specify an option "Allow Multiple Values". This option will allow us to have multiple values for an EA like below,

EAOne = ["valOne", "valTwo"]

But with this present implementation we can have only one value for an EA. Can you enhance it ?
Code at https://github.com/infobloxopen/terraform-provider-infoblox/blob/develop/infoblox/resource_infoblox_network.go#L93 would help you in this direction.

We can use jsonencode function to write the apt tf files https://www.terraform.io/docs/language/functions/jsonencode.html

@AliAllomani
Copy link
Author

@somashekhar MR has been updated.

example usage

resource "infoblox_ip_allocation" "foo"{
	network_view_name="default"
	vm_name="test-name"
	cidr="10.0.0.0/24"
	tenant_id="foo"

	extattr {
		name = "Site"
		value = "Site1"
	}

	extattr {
		name = "TestList"
		values = [
			"Val1",
			"Val2"
		]
	}
	
}

Optional: true,
},
"values": {
Type: schema.TypeList,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map allows as of now only TypeBool, TypeInt, TypeFloat and TypeString as possible value data types https://github.com/hashicorp/terraform/blob/6697245c18dad6848e7647598dda1ab04c2680ca/internal/legacy/helper/schema/schema.go#L1605 and https://github.com/hashicorp/terraform/blob/v0.14.7/helper/schema/schema.go#L169

I would expect the plugin would crash at run time though it does not give a compilation error.

To overcome this you can use jsonencode at tf file and then unmarshal it at code end https://github.com/infobloxopen/terraform-provider-infoblox/blob/develop/infoblox/resource_infoblox_network.go#L93

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@somashekhar i don't understand your point

I would expect the plugin would crash at run time though it does not give a compilation error.

This is why we have acceptance tests, and it's passing it.

Regarding

Map allows as of now only TypeBool, TypeInt, TypeFloat and TypeString as possible value data types

This is *Resource not TypeMap

If it is *Resource, the element type is a complex structure,
https://github.com/hashicorp/terraform/blob/fce896d6df314a2c77a9daecb5c61d8beea157db/helper/schema/schema.go#L173

Regarding

To overcome this you can use jsonencode at tf file and then unmarshal it at code end

in my opinion, I don't think setting the values as json content in tf is a recommended way, the users should have a simple way to define the parameters values.

the way you're suggesting will do marshal (tf code using jsonencode) , then unmarshal (resource), then marshal again (client side) to send the request using rest api

@jtcarnes
Copy link

Any movement on this issue? I was looking to do this myself, but happy to support this MR as needed

@somashekhar
Copy link
Contributor

@jtcarnes we have provided extensible attributes support (along with custom EAs) under develop branch. Do have a look.
Since an internal development was already in progress with an different approach, we could not imbibe this approach in the mid of development. But we would be reviewing and considering this approach of defining EAs in the coming days.

@AliAllomani Holding on this PR merge since there is already an approach for defining EAs is in place. Would surely propose this approach to team.

@somashekhar somashekhar changed the base branch from develop to master August 9, 2021 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants