Add VPC data source #1207
Add VPC data source #1207
Conversation
|
/test-all |
ksamoray
force-pushed
the
ds-vpc
branch
2 times, most recently
from
cddacce
to
9cb2338
Compare
|
/test-all |
nsxt/data_source_nsxt_policy_vpc.go
Outdated
| return fmt.Errorf("error obtaining VPC ID or name during read") | ||
| } else { | ||
| // Get by full name/prefix | ||
| objList, err := client.List(defaultOrgID, ctx.ProjectID, nil, nil, nil, nil, nil, nil) |
There was a problem hiding this comment.
Search - to the best of my knowledge - should be working across orgs and projects.
Can we use it here instead of iterating over the list?
There was a problem hiding this comment.
I've tried to implement the project datasource with search API at the time, which didn't work. So I didn't even try with this one - maybe it's worthwhile trying.
There was a problem hiding this comment.
Should this support pagination?
There was a problem hiding this comment.
Replaced logic with search API
nsxt/data_source_nsxt_policy_vpc.go
Outdated
| } | ||
| } | ||
| if len(perfectMatch) > 0 { | ||
| if len(perfectMatch) > 1 { |
There was a problem hiding this comment.
For VPC objects perhaps uniqueness should be defined within a project.
Perhaps we should filter by ctx.projectId and display name.
There was a problem hiding this comment.
client.List() call (L55) takes a project parameter, so uniqueness is only checked at the scope of these query results.
nsxt/data_source_policy_vpc_test.go
Outdated
| resource.TestCheckResourceAttr(testResourceName, "display_name", name), | ||
| resource.TestCheckResourceAttr(testResourceName, "description", name), | ||
| resource.TestCheckResourceAttrSet(testResourceName, "path"), | ||
| //resource.TestCheckResourceAttrSet(testResourceName, "default_gateway_path"), |
There was a problem hiding this comment.
Why is this attribute commented? Is it giving you troubles?
There was a problem hiding this comment.
I didn't set the DGW path parameter in Create() (L84), I intended to remove this as although I could gather the T0 id, I had to create a path from it as I do with IP block. It's quite ugly... If you think that there's some importance to this, I can add it.
TBH I'd rather rewrite this once we have a resource for VPC.
nsxt/data_source_nsxt_policy_vpc.go
Outdated
| "display_name": getDataSourceDisplayNameSchema(), | ||
| "description": getDataSourceDescriptionSchema(), | ||
| "path": getPathSchema(), | ||
| "context": getContextSchema(), |
There was a problem hiding this comment.
Should project be required here?
|
/test-all |
| * `site_info` - Information related to sites applicable for given Project. | ||
| * `edge_cluster_paths` - The edge cluster on which the networking elements for the Org will be created. | ||
| * `site_path` - This represents the path of the site which is managed by Global Manager. For the local manager, if set, this needs to point to 'default'. | ||
| * `default_gateway_path` - The tier 0 has to be pre-created before Project is created. |
There was a problem hiding this comment.
This description does not seem relevant to data source
| * `path` - The NSX path of the policy resource. | ||
| * `short_id` - Defaults to id if id is less than equal to 8 characters or defaults to random generated id if not set. | ||
| * `site_info` - Information related to sites applicable for given Project. | ||
| * `edge_cluster_paths` - The edge cluster on which the networking elements for the Org will be created. |
| * `short_id` - Defaults to id if id is less than equal to 8 characters or defaults to random generated id if not set. | ||
| * `site_info` - Information related to sites applicable for given Project. | ||
| * `edge_cluster_paths` - The edge cluster on which the networking elements for the Org will be created. | ||
| * `site_path` - This represents the path of the site which is managed by Global Manager. For the local manager, if set, this needs to point to 'default'. |
There was a problem hiding this comment.
I'm not sure this is relevant for VPC
There was a problem hiding this comment.
I'm not sure either - I reused the SiteInfo from Project as these two use the same schema. Should I remove site_path entirely from this DS?
There was a problem hiding this comment.
@tvigneron could you please give your opinion?
|
|
||
| * `id` - (Optional) The ID of VPC to retrieve. If ID is specified, no additional argument should be configured. | ||
| * `display_name` - (Optional) The Display Name prefix of the VPC to retrieve. | ||
| * `context` - (Optional) The context which the object belongs to |
| "default_gateway_path": getPathSchema(), | ||
| "short_id": { | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
According to the code this is fully computed, in which case Optional: true should be removed, so that terraform does not allow user to specify it
There was a problem hiding this comment.
This value is assigned by NSX if not set by user, and therefore has to be computed. It can also be set by the user (it should be unique - NSX will enforce that).
The UI doesn't allow updating it (on create there's the text "This field can be set at VPC creation time and cannot be edited later. Max 8 characters allowed.").
BTW there's an open issue for this in the Project.
Signed-off-by: Kobi Samoray <kobi.samoray@broadcom.com>
Signed-off-by: Kobi Samoray <kobi.samoray@broadcom.com>
|
/test-all |
No description provided.