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.

Sign up for GitHub

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

Jump to bottom

Add Virtual Machine Scale Set Data Source #7141

Merged
merged 6 commits into from
Jun 4, 2020
Merged

Add Virtual Machine Scale Set Data Source #7141

merged 6 commits into from
Jun 4, 2020

Conversation

nickmhankins
Copy link
Contributor

@nickmhankins nickmhankins commented May 29, 2020 •
edited
Loading

Exports the scale set ID as well as an identity block

=== PAUSE TestAccDataSourceAzureRMVirtualMachine_basicLinux
=== RUN   TestAccDataSourceAzureRMVirtualMachine_basicWindows
=== PAUSE TestAccDataSourceAzureRMVirtualMachine_basicWindows
=== RUN   TestAccDataSourceAzureRMVirtualMachineScaleSet_basicLinux
=== PAUSE TestAccDataSourceAzureRMVirtualMachineScaleSet_basicLinux
=== RUN   TestAccDataSourceAzureRMVirtualMachineScaleSet_basicWindows
=== PAUSE TestAccDataSourceAzureRMVirtualMachineScaleSet_basicWindows
=== CONT  TestAccDataSourceAzureRMVirtualMachine_basicLinux
=== CONT  TestAccDataSourceAzureRMVirtualMachineScaleSet_basicWindows
=== CONT  TestAccDataSourceAzureRMVirtualMachine_basicWindows
=== CONT  TestAccDataSourceAzureRMVirtualMachineScaleSet_basicLinux
--- PASS: TestAccDataSourceAzureRMVirtualMachine_basicWindows (358.42s)
--- PASS: TestAccDataSourceAzureRMVirtualMachine_basicLinux (452.13s)
--- PASS: TestAccDataSourceAzureRMVirtualMachineScaleSet_basicLinux (507.31s)
--- PASS: TestAccDataSourceAzureRMVirtualMachineScaleSet_basicWindows (546.94s)
PASS

Fixes #7142

@jackofallops jackofallops added new-data-source service/vmss Virtual Machine Scale Sets labels Jun 1, 2020
ArcturusZhang
ArcturusZhang suggested changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

Hi @nickmhankins thanks for opening this PR! Overall this PR looks good, aside from some minor comments I have left inline.

azurerm/internal/services/compute/virtual_machine_scale_set_data_source.go
return fmt.Errorf("Error making Read request on Virtual Machine Scale Set %q (Resource Group %q): %+v", name, resGroup, err)
}

d.SetId(*resp.ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we first check if the ID is non-nil and non-empty before actually set the ID? Otherwise this would give the user a panic (if nil) or unexpected result (empty ID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, added

website/docs/d/virtual_machine_scale_set.html.markdown Outdated

* `principal_id` - The ID of the System Managed Service Principal assigned to the Virtual Machine Scale Set.

* `type` - The identity types of the Managed Identity assigned to the Virtual Machine Scale Set.
Copy link
Contributor

Choose a reason for hiding this comment

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

A minor typo

Suggested change
* `type` - The identity types of the Managed Identity assigned to the Virtual Machine Scale Set.
* `type` - The identity type of the Managed Identity assigned to the Virtual Machine Scale Set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

azurerm/internal/services/compute/tests/virtual_machine_scale_set_data_source_test.go
},
})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add a test case for the azurerm_orchestrated_virtual_machine_scale_set? I suppose it would also be useful.

Copy link
Contributor Author

@nickmhankins nickmhankins Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ArcturusZhang Does the Orchestrated Scale Set resource set export those identity properties? I'm not sure it would be possible if not

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, orchestrated VMSS does not support the identity yet, therefore if you import an orchestrated VMSS into this data source, the identity block would be empty.

Copy link
Contributor Author

@nickmhankins nickmhankins Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ArcturusZhang Got it, test added.

@nickmhankins
Copy link
Contributor Author

The tests have all finished and passed, not sure why it still says pending on GitHub.

ArcturusZhang
ArcturusZhang approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

LGTM 👍

tombuildsstuff
tombuildsstuff approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this @nickmhankins

@tombuildsstuff tombuildsstuff added this to the v2.13.0 milestone Jun 4, 2020
@tombuildsstuff
Copy link
Contributor

LGTM - looks like an issue between Travis <-> Github but it's green there:

Screenshot 2020-06-04 at 11 43 09

@tombuildsstuff tombuildsstuff merged commit 04cfd75 into hashicorp:master Jun 4, 2020
tombuildsstuff added a commit that referenced this pull request Jun 4, 2020
@tombuildsstuff
updating to include #7141
0830fa9
@ghost
Copy link

ghost commented Jun 4, 2020

This has been released in version 2.13.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.13.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jul 4, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

@ArcturusZhang ArcturusZhang ArcturusZhang approved these changes

Assignees
No one assigned
Labels
documentation new-data-source service/vmss Virtual Machine Scale Sets size/L
Projects
None yet
Milestone
v2.13.0
Development

Successfully merging this pull request may close these issues.

Support for Virtual Machine Scale set data source
5 participants
@nickmhankins @tombuildsstuff @ArcturusZhang @jackofallops @automatetheneedful