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

Add vcd_ipset resource and data source #406

Merged
merged 12 commits into from
Nov 29, 2019
Merged

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Nov 27, 2019

This PR introduces IP set resource and data source (vcd_ipset) for handling IP sets.
At the moment there is no much use of them, but soon PRs will come in to enable support in vcd_nsxv_firewall_rule resource and later on DHCP pool handling in edge gateway (will need additional new resources)

Note It bumps terraform-provider-sdk version 1.0.0 -> 1.3.0 therefore there are quite a lot of changes in vendor

@Didainius Didainius self-assigned this Nov 27, 2019
@ghost ghost added the size/XXL label Nov 27, 2019
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

First pass. More follow after tests


## Attribute Reference

All the attributes defined in `vcd_ipset` resource are available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a link to the resource

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added


Supported in provider *v2.6+*

## Example Usage 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an example 2 coming?
I'd like to see several examples of different combinations allowed in ip_addresses and at least one example of how to use this in other resources, even if they don't exist yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will come either in IP set or in vcd_nsxv_firewall_rule when there is a PR to start supporting it.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Small asks. BTW, it's really good that this comes as a bundle of resource, data source, read and import - we should always strive for that.

* `name` - (Required) Unique IP set name.
* `description` - (Optional) An optional description for IP set.
* `ip_addresses` - (Required) A set of IP addresses, CIDRs and ranges as strings.
* `is_inheritance_allowed` (Optional) Toggle to enable inheritance to allow visibility at underlying scopes. (Default `true`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency:

Suggested change
* `is_inheritance_allowed` (Optional) Toggle to enable inheritance to allow visibility at underlying scopes. (Default `true`)
* `is_inheritance_allowed` (Optional) Toggle to enable inheritance to allow visibility at underlying scopes. Default `true`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

~> **Note:** The current implementation of Terraform import can only import resources into the state.
It does not generate configuration. [More information.](https://www.terraform.io/docs/import/)

An existing load balancer application rule can be [imported][docs-import] into this resource
Copy link
Collaborator

Choose a reason for hiding this comment

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

Load balancer? :)

Copy link
Collaborator Author

@Didainius Didainius Nov 28, 2019

Choose a reason for hiding this comment

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

Fixed. The copy/paste thing tricked me although I tried to avoid it.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

LGTM. Though I would like SDK upgrade as separate PR

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

LGTM!

@Didainius
Copy link
Collaborator Author

LGTM. Though I would like SDK upgrade as separate PR

Message noted for future. Although now all the tests were done in this version so I don't want to roll it back and redo all the tests as we willl will bump it anyway.

@Didainius Didainius merged commit d4eaebc into vmware:master Nov 29, 2019
@Didainius Didainius deleted the ipsets branch November 29, 2019 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants