-
Notifications
You must be signed in to change notification settings - Fork 84
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
VPC Groups resource implementation #1231
Conversation
resource.TestCheckResourceAttr(testResourceName, "criteria.#", "1"), | ||
resource.TestCheckResourceAttr(testResourceName, "criteria.0.ipaddress_expression.#", "1"), | ||
resource.TestCheckResourceAttr(testResourceName, "criteria.0.ipaddress_expression.0.ip_addresses.#", "1"), | ||
resource.TestCheckResourceAttr(testResourceName, "criteria.1.macaddress_expression.#", "0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add to the test more criteria, such as those on subnet and subnet ports?
subcategory: "VPC" | ||
layout: "nsxt" | ||
page_title: "NSXT: nsxt_vpc_group" | ||
description: A resource to configure a VPC Group and its members. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should it work here? Do we need to link to the policy_group resource page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That requires completion
{ | ||
Config: testAccNsxtVPCGroupAddressCreateTemplate(name, resourceName, true), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccNsxtPolicyGroupExists(testResourceName, defaultDomain), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add context validation to one of the tests
* `description` - (Optional) Description of the resource. | ||
* `tag` - (Optional) A list of scope + tag pairs to associate with this Group. | ||
* `nsx_id` - (Optional) The NSX ID of this resource. If set, this ID will be used to create the group resource. | ||
* `context` - (Optional) The context which the object belongs to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is marked as Required
in vpc security policy docs.. This will change when provider supports the context clause, just a reminder to go over the docs and change appropriately if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as PR #1226 isn't merged, it's better if we keep things consistent with the code. I'll change that.
LGTM, just some minor comments |
fdf0249
to
4380076
Compare
/test-all |
Signed-off-by: Kobi Samoray <[email protected]>
/test-all |
Update: resourceNsxtVPCGroupUpdate, | ||
Delete: resourceNsxtVPCGroupDelete, | ||
Importer: &schema.ResourceImporter{ | ||
State: nsxtPolicyPathResourceImporter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not specific to group, but rather to all vpc resources - it seems to me that we need VPC-specific Importer that would verify that the path is VPC path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, call nsxtPolicyPathResourceImporterHelper()
and fail if no project_id
or vpc_id
are specified? Yeah that could be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, a wrapper for nsxtPolicyPathResourceImporter
that also verifies we have a VPC path. I don't think we should validate the object ids against what we might have and not have in config.
maybe its a good idea to create a generic importer that verifies specified path structure, and vpc would be just a use case for that. This can be a follow up if you prefer.
No description provided.