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

Set devices parameter as optional for vsphere_distributed_switch resource #1468

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

haroldddd
Copy link
Contributor

@haroldddd haroldddd commented Aug 23, 2021

Description

Updates the structure of distributed_virtual_switch: setting devices argument from required to optional.
By default, vCenter does not force a distributed-virtual-switch to be bound to any devices. This was a requirement introduced by terraform-vsphere-provider.

Release Note

Release note for CHANGELOG:

resource/distributed_virtual_switch - devices argument is now optional [GH-1468]

References

Closes #1290

vCenter does not forces a distributed virtual-switch to have any devices bound.
This patch fixes a requirements introduced by terraform-vsphere-provider thus technically non-mandatory.
@hashicorp-cla
Copy link

hashicorp-cla commented Aug 23, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the size/xs Relative Sizing: Extra-Small label Aug 23, 2021
@tenthirtyam
Copy link
Collaborator

Ideally, the docs should be updated to reflect optional as part of the PR.

Ryan

Updated the documentation to match with commit b60361b
By default, vCenter does not forces a distributed-virtual-switch to be bound to any devices. This was a requirement introduced by terraform-vsphere-provider.
@github-actions github-actions bot added the documentation Type: Documentation label Nov 17, 2021
@haroldddd
Copy link
Contributor Author

You're right. I've just updated the documentation to stay consistent with the code.

@haroldddd
Copy link
Contributor Author

Any news on that one ?

@tenthirtyam
Copy link
Collaborator

@appilon, this pull request makes sense to review to relax requirement by the provider that differs from the API and UI methods.

Ryan

@appilon
Copy link
Contributor

appilon commented Jan 24, 2022

Going from required --> optional should be okay from a breaking change perspective. The PR will need to be rebased still (which I can handle) but I would like to see the tests for this resource be ran:

➜ make testacc TESTARGS='-run=TestAccResourceVSphereDistributedVirtualSwitch_'

Unfortunately I am without a test environment at the moment but I should be able to get one back up hopefully soon. If someone does have an environment configured for running tests (you will need to set several environment variables) feel free to post the results.

@tenthirtyam
Copy link
Collaborator

@haroldddd Can you update the description summary to include "Closes #1290" to link the issue and pull request?

Thanks,
Ryan

@tenthirtyam tenthirtyam added the enhancement Type: Enhancement label Feb 3, 2022
@tenthirtyam tenthirtyam changed the title Set devices parameter as non-mandatory for DVS Set devices parameter as non-mandatory for vsphere_distributed_switch resource Feb 3, 2022
@tenthirtyam tenthirtyam changed the title Set devices parameter as non-mandatory for vsphere_distributed_switch resource Set devices parameter as non-mandatory for vsphere_distributed_switch resource Feb 3, 2022
@tenthirtyam tenthirtyam changed the title Set devices parameter as non-mandatory for vsphere_distributed_switch resource Set devices parameter as optional for vsphere_distributed_switch resource Feb 3, 2022
@tenthirtyam tenthirtyam added the needs-review Status: Pull Request Needs Review label Feb 8, 2022
@github-actions github-actions bot added provider Type: Provider and removed documentation Type: Documentation labels Feb 8, 2022
@tenthirtyam
Copy link
Collaborator

@appilon - would you mind rebasing this one for me and I'll start to take a look into it.

Thanks!
Ryan

@tenthirtyam tenthirtyam self-requested a review February 10, 2022 00:44
@appilon
Copy link
Contributor

appilon commented Feb 10, 2022

@tenthirtyam When we merge I will perform a squash and merge (commits get squashed to 1, the conflict was in an intermediary commit which won't exist after squash) so no need to rebase at all. Could you do a quick test where you create a vsphere_distributed_switch without any devices and ensures it creates and destroys with no error?

@tenthirtyam
Copy link
Collaborator

@appilon, certainly! Stay tuned.

- Correct formatting for `Optional: true,`.
- Move the `Description` below `Optional`.

Signed-off-by: Ryan Johnson <[email protected]>
Updates devices as optional for the `host` arguments in the `vsphere_distributed_virtual_switch` resource docs.

Signed-off-by: Ryan Johnson <[email protected]>
@github-actions github-actions bot added the documentation Type: Documentation label Feb 10, 2022
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

LGTM.

I've reviewed the changes and made some minor edits:

  • Corrected formatting/indentation for Optional: true,. Would have been corrected with go fmt.
  • Move the Description below Optional like the rest of the schema.
  • Updates devices as optional for the host arguments in the vsphere_distributed_virtual_switch resource docs.

My tests passed using the example below which attaches a host to the VDS but with no devices defined for the host(s).

resource "vsphere_distributed_virtual_switch" "vds" {
  name          = "terraform-test-dvs"
  datacenter_id = data.vsphere_datacenter.dc.id

  uplinks         = ["uplink1", "uplink2", "uplink3", "uplink4"]
  active_uplinks  = ["uplink1", "uplink2"]
  standby_uplinks = ["uplink3", "uplink4"]

  network_resource_control_enabled = true

  host {
    host_system_id = data.vsphere_host.host.id
  }

}

Results

Apply

terraform apply  -auto-approve

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # vsphere_distributed_virtual_switch.vds will be created
  + resource "vsphere_distributed_virtual_switch" "vds" {
      + active_uplinks                    = [
          + "uplink1",
          + "uplink2",
        ]
      + allow_forged_transmits            = (known after apply)
      + allow_mac_changes                 = (known after apply)
      + allow_promiscuous                 = (known after apply)
      + backupnfc_maximum_mbit            = (known after apply)
      + backupnfc_reservation_mbit        = (known after apply)
      + backupnfc_share_count             = (known after apply)
      + backupnfc_share_level             = (known after apply)
      + block_all_ports                   = (known after apply)
      + check_beacon                      = (known after apply)
      + config_version                    = (known after apply)
      + datacenter_id                     = "datacenter-3"
      + directpath_gen2_allowed           = (known after apply)
      + egress_shaping_average_bandwidth  = (known after apply)
      + egress_shaping_burst_size         = (known after apply)
      + egress_shaping_enabled            = (known after apply)
      + egress_shaping_peak_bandwidth     = (known after apply)
      + failback                          = (known after apply)
      + faulttolerance_maximum_mbit       = (known after apply)
      + faulttolerance_reservation_mbit   = (known after apply)
      + faulttolerance_share_count        = (known after apply)
      + faulttolerance_share_level        = (known after apply)
      + hbr_maximum_mbit                  = (known after apply)
      + hbr_reservation_mbit              = (known after apply)
      + hbr_share_count                   = (known after apply)
      + hbr_share_level                   = (known after apply)
      + id                                = (known after apply)
      + ignore_other_pvlan_mappings       = false
      + ingress_shaping_average_bandwidth = (known after apply)
      + ingress_shaping_burst_size        = (known after apply)
      + ingress_shaping_enabled           = (known after apply)
      + ingress_shaping_peak_bandwidth    = (known after apply)
      + iscsi_maximum_mbit                = (known after apply)
      + iscsi_reservation_mbit            = (known after apply)
      + iscsi_share_count                 = (known after apply)
      + iscsi_share_level                 = (known after apply)
      + lacp_api_version                  = (known after apply)
      + lacp_enabled                      = (known after apply)
      + lacp_mode                         = (known after apply)
      + link_discovery_operation          = "listen"
      + link_discovery_protocol           = "cdp"
      + management_maximum_mbit           = (known after apply)
      + management_reservation_mbit       = (known after apply)
      + management_share_count            = (known after apply)
      + management_share_level            = (known after apply)
      + max_mtu                           = (known after apply)
      + multicast_filtering_mode          = (known after apply)
      + name                              = "terraform-test-dvs"
      + netflow_active_flow_timeout       = 60
      + netflow_enabled                   = (known after apply)
      + netflow_idle_flow_timeout         = 15
      + network_resource_control_enabled  = true
      + network_resource_control_version  = (known after apply)
      + nfs_maximum_mbit                  = (known after apply)
      + nfs_reservation_mbit              = (known after apply)
      + nfs_share_count                   = (known after apply)
      + nfs_share_level                   = (known after apply)
      + notify_switches                   = (known after apply)
      + port_private_secondary_vlan_id    = (known after apply)
      + standby_uplinks                   = [
          + "uplink3",
          + "uplink4",
        ]
      + teaming_policy                    = (known after apply)
      + tx_uplink                         = (known after apply)
      + uplinks                           = [
          + "uplink1",
          + "uplink2",
          + "uplink3",
          + "uplink4",
        ]
      + vdp_maximum_mbit                  = (known after apply)
      + vdp_reservation_mbit              = (known after apply)
      + vdp_share_count                   = (known after apply)
      + vdp_share_level                   = (known after apply)
      + version                           = (known after apply)
      + virtualmachine_maximum_mbit       = (known after apply)
      + virtualmachine_reservation_mbit   = (known after apply)
      + virtualmachine_share_count        = (known after apply)
      + virtualmachine_share_level        = (known after apply)
      + vlan_id                           = (known after apply)
      + vmotion_maximum_mbit              = (known after apply)
      + vmotion_reservation_mbit          = (known after apply)
      + vmotion_share_count               = (known after apply)
      + vmotion_share_level               = (known after apply)
      + vsan_maximum_mbit                 = (known after apply)
      + vsan_reservation_mbit             = (known after apply)
      + vsan_share_count                  = (known after apply)
      + vsan_share_level                  = (known after apply)

      + host {
          + devices        = []
          + host_system_id = "host-10"
        }

      + vlan_range {
          + max_vlan = (known after apply)
          + min_vlan = (known after apply)
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
vsphere_distributed_virtual_switch.vds: Creating...
vsphere_distributed_virtual_switch.vds: Creation complete after 1s [id=50 02 63 0f 25 36 ca 35-62 86 ae 10 8e 31 5f c5]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Notice in the above:

      + host {
          + devices        = []
          + host_system_id = "host-10"
        }

The VDS was successfully created, one host attached but no devices from the host.

The destroy was also sucessful:

terraform destroy --auto-approve
vsphere_distributed_virtual_switch.vds: Refreshing state... [id=50 02 63 0f 25 36 ca 35-62 86 ae 10 8e 31 5f c5]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  - destroy

Terraform will perform the following actions:

  # vsphere_distributed_virtual_switch.vds will be destroyed
  - resource "vsphere_distributed_virtual_switch" "vds" {
      - active_uplinks                    = [
          - "uplink1",
          - "uplink2",
        ] -> null
      - allow_forged_transmits            = false -> null
      - allow_mac_changes                 = false -> null
      - allow_promiscuous                 = false -> null
      - backupnfc_maximum_mbit            = -1 -> null
      - backupnfc_reservation_mbit        = 0 -> null
      - backupnfc_share_count             = 50 -> null
      - backupnfc_share_level             = "normal" -> null
      - block_all_ports                   = false -> null
      - check_beacon                      = false -> null
      - config_version                    = "5" -> null
      - custom_attributes                 = {} -> null
      - datacenter_id                     = "datacenter-3" -> null
      - directpath_gen2_allowed           = false -> null
      - egress_shaping_average_bandwidth  = 100000000 -> null
      - egress_shaping_burst_size         = 104857600 -> null
      - egress_shaping_enabled            = false -> null
      - egress_shaping_peak_bandwidth     = 100000000 -> null
      - failback                          = true -> null
      - faulttolerance_maximum_mbit       = -1 -> null
      - faulttolerance_reservation_mbit   = 0 -> null
      - faulttolerance_share_count        = 50 -> null
      - faulttolerance_share_level        = "normal" -> null
      - hbr_maximum_mbit                  = -1 -> null
      - hbr_reservation_mbit              = 0 -> null
      - hbr_share_count                   = 50 -> null
      - hbr_share_level                   = "normal" -> null
      - id                                = "50 02 63 0f 25 36 ca 35-62 86 ae 10 8e 31 5f c5" -> null
      - ignore_other_pvlan_mappings       = false -> null
      - ingress_shaping_average_bandwidth = 100000000 -> null
      - ingress_shaping_burst_size        = 104857600 -> null
      - ingress_shaping_enabled           = false -> null
      - ingress_shaping_peak_bandwidth    = 100000000 -> null
      - iscsi_maximum_mbit                = -1 -> null
      - iscsi_reservation_mbit            = 0 -> null
      - iscsi_share_count                 = 50 -> null
      - iscsi_share_level                 = "normal" -> null
      - lacp_api_version                  = "multipleLag" -> null
      - lacp_enabled                      = false -> null
      - lacp_mode                         = "passive" -> null
      - link_discovery_operation          = "listen" -> null
      - link_discovery_protocol           = "cdp" -> null
      - management_maximum_mbit           = -1 -> null
      - management_reservation_mbit       = 0 -> null
      - management_share_count            = 50 -> null
      - management_share_level            = "normal" -> null
      - max_mtu                           = 1500 -> null
      - multicast_filtering_mode          = "snooping" -> null
      - name                              = "terraform-test-dvs" -> null
      - netflow_active_flow_timeout       = 60 -> null
      - netflow_collector_port            = 0 -> null
      - netflow_enabled                   = false -> null
      - netflow_idle_flow_timeout         = 15 -> null
      - netflow_internal_flows_only       = false -> null
      - netflow_observation_domain_id     = 0 -> null
      - netflow_sampling_rate             = 0 -> null
      - network_resource_control_enabled  = true -> null
      - network_resource_control_version  = "version3" -> null
      - nfs_maximum_mbit                  = -1 -> null
      - nfs_reservation_mbit              = 0 -> null
      - nfs_share_count                   = 50 -> null
      - nfs_share_level                   = "normal" -> null
      - notify_switches                   = true -> null
      - standby_uplinks                   = [
          - "uplink3",
          - "uplink4",
        ] -> null
      - tags                              = [] -> null
      - teaming_policy                    = "loadbalance_srcid" -> null
      - tx_uplink                         = false -> null
      - uplinks                           = [
          - "uplink1",
          - "uplink2",
          - "uplink3",
          - "uplink4",
        ] -> null
      - vdp_maximum_mbit                  = -1 -> null
      - vdp_reservation_mbit              = 0 -> null
      - vdp_share_count                   = 50 -> null
      - vdp_share_level                   = "normal" -> null
      - version                           = "7.0.3" -> null
      - virtualmachine_maximum_mbit       = -1 -> null
      - virtualmachine_reservation_mbit   = 0 -> null
      - virtualmachine_share_count        = 100 -> null
      - virtualmachine_share_level        = "high" -> null
      - vlan_id                           = 0 -> null
      - vmotion_maximum_mbit              = -1 -> null
      - vmotion_reservation_mbit          = 0 -> null
      - vmotion_share_count               = 50 -> null
      - vmotion_share_level               = "normal" -> null
      - vsan_maximum_mbit                 = -1 -> null
      - vsan_reservation_mbit             = 0 -> null
      - vsan_share_count                  = 50 -> null
      - vsan_share_level                  = "normal" -> null

      - host {
          - devices        = [] -> null
          - host_system_id = "host-10" -> null
        }
    }

Plan: 0 to add, 0 to change, 1 to destroy.
vsphere_distributed_virtual_switch.vds: Destroying... [id=50 02 63 0f 25 36 ca 35-62 86 ae 10 8e 31 5f c5]
vsphere_distributed_virtual_switch.vds: Destruction complete after 2s

Destroy complete! Resources: 1 destroyed.

@tenthirtyam tenthirtyam assigned appilon and unassigned appilon and tenthirtyam Feb 10, 2022
@appilon appilon merged commit 9f5a71c into hashicorp:master Feb 11, 2022
@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Feb 11, 2022
@tenthirtyam tenthirtyam added this to the v2.1.0 milestone Feb 14, 2022
@github-actions
Copy link

github-actions bot commented Mar 2, 2022

This functionality has been released in v2.1.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Apr 2, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Type: Documentation enhancement Type: Enhancement provider Type: Provider size/xs Relative Sizing: Extra-Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax devices as a required argument for vsphere_distributed_switch resource
4 participants