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

New resource: data azurerm_locations #23324

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

HappyTobi
Copy link
Contributor

Hi all,

I created a new "data" resource called azurerm_locations to close the issue #3025

Regards
Tobi

@HappyTobi
Copy link
Contributor Author

Hi all,

@tombuildsstuff do you think you can take a look?

Thank you
Regards

@HappyTobi
Copy link
Contributor Author

Hi @tombuildsstuff do you have a chance to review my pr?

@ms-zhenhua
Copy link
Contributor

ms-zhenhua commented Nov 10, 2023

Hi @HappyTobi, thank you for contributing this PR. According to the latest Contributor Guide, the provider accepts Typed resources only. Could you kindly update your code to a Typed resource, then we can have another look. You may use this PR as an example.
BTW, please also attach your local passed test result in the description or comments. Thanks.

@HappyTobi
Copy link
Contributor Author

@ms-zhenhua can you please review the implementation again.

Test output:

make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceSubscription_current' TESTTIMEOUT='60m' 
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceSubscription_current -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceSubscription_current
=== PAUSE TestAccDataSourceSubscription_current
=== CONT  TestAccDataSourceSubscription_current
--- PASS: TestAccDataSourceSubscription_current (13.82s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/subscription  13.845s

@ms-zhenhua
Copy link
Contributor

@ms-zhenhua can you please review the implementation again.

Test output:

make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceSubscription_current' TESTTIMEOUT='60m' 
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceSubscription_current -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceSubscription_current
=== PAUSE TestAccDataSourceSubscription_current
=== CONT  TestAccDataSourceSubscription_current
--- PASS: TestAccDataSourceSubscription_current (13.82s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/subscription  13.845s

Hi @HappyTobi, thank you for your updates. But TestAccDataSourceSubscription_current is not the testcases which you added this time. Could you provide the test results of the testcases added in this PR?
BTW, since Terraform natively supports filtering, do we still need to add the filter property?

@HappyTobi
Copy link
Contributor Author

Hi @ms-zhenhua
Thanks for the feedback.
I removed the filter, I think simpler and less code is much better than a "data source" filter.

Here is also the test output after the code was updated:

make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceLocations_westUS' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceLocations_westUS -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceLocations_westUS
=== PAUSE TestAccDataSourceLocations_westUS
=== CONT  TestAccDataSourceLocations_westUS
--- PASS: TestAccDataSourceLocations_westUS (30.64s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/subscription	32.379s

other test:

make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceLocations_NonExistingRegion' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceLocations_NonExistingRegion -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceLocations_NonExistingRegion
=== PAUSE TestAccDataSourceLocations_NonExistingRegion
=== CONT  TestAccDataSourceLocations_NonExistingRegion
--- PASS: TestAccDataSourceLocations_NonExistingRegion (15.77s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/subscription	17.490s

Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

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

Hi @HappyTobi , I've taken a look through and left some comments inline. Kindly confirm. Once that's done I can take another look over. Thanks.

internal/services/subscription/client/client.go Outdated Show resolved Hide resolved
internal/services/subscription/client/client.go Outdated Show resolved Hide resolved
internal/services/subscription/client/client.go Outdated Show resolved Hide resolved
internal/services/subscription/data_source_locations.go Outdated Show resolved Hide resolved
internal/services/subscription/data_source_locations.go Outdated Show resolved Hide resolved
website/docs/d/locations.html.markdown Outdated Show resolved Hide resolved
website/docs/d/locations.html.markdown Outdated Show resolved Hide resolved
website/docs/d/locations.html.markdown Outdated Show resolved Hide resolved
website/docs/d/locations.html.markdown Outdated Show resolved Hide resolved
website/docs/d/locations.html.markdown Outdated Show resolved Hide resolved
@HappyTobi
Copy link
Contributor Author

Hi @ms-zhenhua
I updated the implementation and run the tests again:

make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceLocations_westUS' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceLocations_westUS -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceLocations_westUS
=== PAUSE TestAccDataSourceLocations_westUS
=== CONT  TestAccDataSourceLocations_westUS
--- PASS: TestAccDataSourceLocations_westUS (29.20s)
PASS
make acctests SERVICE='subscription' TESTARGS='-run=TestAccDataSourceLocations_westUS' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/subscription -run=TestAccDataSourceLocations_westUS -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceLocations_westUS
=== PAUSE TestAccDataSourceLocations_westUS
=== CONT  TestAccDataSourceLocations_westUS
--- PASS: TestAccDataSourceLocations_westUS (29.20s)
PASS

@ms-zhenhua
Copy link
Contributor

@HappyTobi Thank you for your updates. Could you kindly confirm the following testcase error?

image

@HappyTobi
Copy link
Contributor Author

@HappyTobi Thank you for your updates. Could you kindly confirm the following testcase error?

image

I run that test multiple times and didn't see that issue.
I will update the test to just check that the "physical_zone" is not empty.

@ms-zhenhua
Copy link
Contributor

@HappyTobi , thanks for your updates. LGTM. Please update the issue title to azurerm_location

image

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Hi @HappyTobi I had a look through this and left a couple of comments mostly around restricting this to retrieving physical locations only since we use the phrase location to refer to physical locations in the provider. Once those are fixed up we can take another look. Thanks!

internal/services/subscription/data_source_location.go Outdated Show resolved Hide resolved
internal/services/subscription/data_source_location.go Outdated Show resolved Hide resolved
internal/services/subscription/data_source_location.go Outdated Show resolved Hide resolved
website/docs/d/location.html.markdown Outdated Show resolved Hide resolved
website/docs/d/location.html.markdown Outdated Show resolved Hide resolved
@HappyTobi
Copy link
Contributor Author

@catriona-m
any updates?

@HappyTobi HappyTobi force-pushed the data_source_location branch from 555cb0c to f4f4542 Compare December 7, 2023 15:29
@HappyTobi HappyTobi force-pushed the data_source_location branch from f4f4542 to 2a8b44e Compare December 7, 2023 15:31
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.

hey @HappyTobi

Thanks for pushing those changes I've left a couple of minor comments inline but if we can fix those up this otherwise LGTM 👍

internal/services/subscription/client/client.go Outdated Show resolved Hide resolved
internal/services/subscription/location_data_source.go Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff added this to the v3.85.0 milestone Dec 7, 2023
HappyTobi and others added 2 commits December 7, 2023 18:10
Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @HappyTobi LGTM 👍

@stephybun stephybun merged commit 0ec9fd4 into hashicorp:main Dec 11, 2023
24 checks passed
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Dec 15, 2023
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.84.0&#34; to
&#34;3.85.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.85.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.85.0&#xA;FEATURES:&#xA;&#xA;*
New Data Source: `azurerm_locations`
([#23324](hashicorp/terraform-provider-azurerm#23324
New Resource: `azurerm_iotcentral_organization`
([#23132](https://github.com/hashicorp/terraform-provider-azurerm/issues/23132))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
provider: support for authenticating using Azure Kubernetes Service
Workload Identity
([#23965](hashicorp/terraform-provider-azurerm#23965
dependencies: updating to `v0.65.0` of
`github.com/hashicorp/go-azure-helpers`
([#24222](hashicorp/terraform-provider-azurerm#24222
dependencies: updating to `v0.20231214.1220802` of
`github.com/hashicorp/go-azure-sdk`
([#24246](hashicorp/terraform-provider-azurerm#24246
dependencies: updating to version `v0.20231214.1160726` of
`github.com/hashicorp/go-azure-sdk`
([#24241](hashicorp/terraform-provider-azurerm#24241
dependencies: update `security/automation` to use
`hashicorp/go-azure-sdk`
([#24156](hashicorp/terraform-provider-azurerm#24156
`dataprotection`: updating to API Version `2023-05-01`
([#24143](hashicorp/terraform-provider-azurerm#24143
`kusto`: removing the remnants of the old Resource ID Parsers now this
uses `hashicorp/go-azure-sdk`
([#24238](hashicorp/terraform-provider-azurerm#24238
Data Source: `azurerm_cognitive_account` - export the `identity` block
([#24214](hashicorp/terraform-provider-azurerm#24214
Data Source: `azurerm_monitor_workspace` - add support for the
`default_data_collection_endpoint_id` and
`default_data_collection_rule_id` properties
([#24153](hashicorp/terraform-provider-azurerm#24153
Data Source: `azurerm_shared_image_gallery` - add support for the
`image_names` property
([#24176](hashicorp/terraform-provider-azurerm#24176
`azurerm_dns_txt_record` - allow up to `4096` characters for the
property `record.value`
([#24169](hashicorp/terraform-provider-azurerm#24169
`azurerm_container_app` - support for the `workload_profile_name`
property
([#24219](hashicorp/terraform-provider-azurerm#24219
`azurerm_container_app` - suppot for the `init_container` block
([#23955](hashicorp/terraform-provider-azurerm#23955
`azurerm_hpc_cache_blob_nfs_target` - support for the
`verification_timer_in_seconds` and `write_back_timer_in_seconds`
properties
([#24207](hashicorp/terraform-provider-azurerm#24207
`azurerm_hpc_cache_nfs_target` - support for the
`verification_timer_in_seconds` and `write_back_timer_in_seconds`
properties
([#24208](hashicorp/terraform-provider-azurerm#24208
`azurerm_linux_web_app` - make `client_secret_setting_name` optional and
conflict with `client_secret_certificate_thumbprint`
([#21834](hashicorp/terraform-provider-azurerm#21834
`azurerm_linux_web_app_slot` - make `client_secret_setting_name`
optional and conflict with `client_secret_certificate_thumbprint`
([#21834](hashicorp/terraform-provider-azurerm#21834
`azurerm_linux_web_app` - fix a bug in `app_settings` where settings
could be lost
([#24221](hashicorp/terraform-provider-azurerm#24221
`azurerm_linux_web_app_slot` - fix a bug in `app_settings` where
settings could be lost
([#24221](hashicorp/terraform-provider-azurerm#24221
`azurerm_log_analytics_workspace` - add support for the
`immediate_data_purge_on_30_days_enabled` property
([#24015](hashicorp/terraform-provider-azurerm#24015
`azurerm_mssql_server` - support for other identity types for the key
vault key
([#24236](hashicorp/terraform-provider-azurerm#24236
`azurerm_machine_learning_datastore_blobstorage` - resource now skips
validation when being created
([#24078](hashicorp/terraform-provider-azurerm#24078
`azurerm_machine_learning_datastore_datalake_gen2` - resource now skips
validation when being created
([#24078](hashicorp/terraform-provider-azurerm#24078
`azurerm_machine_learning_datastore_fileshare` - resource now skips
validation when being created
([#24078](hashicorp/terraform-provider-azurerm#24078
`azurerm_monitor_workspace` - support for the
`default_data_collection_endpoint_id` and
`default_data_collection_rule_id` properties
([#24153](hashicorp/terraform-provider-azurerm#24153
`azurerm_redis_cache` - support for the
`storage_account_subscription_id` property
([#24101](hashicorp/terraform-provider-azurerm#24101
`azurerm_storage_blob` - support for the `source_content` type `Page`
([#24177](hashicorp/terraform-provider-azurerm#24177
`azurerm_web_application_firewall_policy` - support new values to the
`rule_group_name` property
([#24194](hashicorp/terraform-provider-azurerm#24194
`azurerm_windows_web_app` - make the `client_secret_setting_name`
property optional and conflicts with the
`client_secret_certificate_thumbprint` property
([#21834](hashicorp/terraform-provider-azurerm#21834
`azurerm_windows_web_app_slot` - make the `client_secret_setting_name`
property optional and conflicts with the
`client_secret_certificate_thumbprint` property
([#21834](hashicorp/terraform-provider-azurerm#21834
`azurerm_windows_web_app` - fix a bug in `app_settings` where settings
could be lost
([#24221](hashicorp/terraform-provider-azurerm#24221
`azurerm_windows_web_app_slot` - fix a bug in `app_settings` where
settings could be lost
([#24221](hashicorp/terraform-provider-azurerm#24221
`azurerm_cognitive_account` - add `ContentSafety` to the `kind` property
validation
([#24205](https://github.com/hashicorp/terraform-provider-azurerm/issues/24205))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* provider: fix an authentication issue with Azure
Storage when running in Azure China cloud
([#24246](hashicorp/terraform-provider-azurerm#24246
Data Source: `azurerm_role_definition` - fix bug where
`role_definition_id` and `scope` were being incorrectly set
([#24211](hashicorp/terraform-provider-azurerm#24211
`azurerm_batch_account` - fix bug where `UserAssigned, SystemAssigned`
could be passed to the resource even though it isn&#39;t supported
([#24204](hashicorp/terraform-provider-azurerm#24204
`azurerm_batch_pool` - fix bug where `settings_json` and
`protected_settings` were not being unmarshaled
([#24075](hashicorp/terraform-provider-azurerm#24075
`azurerm_bot_service_azure_bot` - fix bug where
`public_network_access_enabled` was being set as the value for `LuisKey`
([#24164](hashicorp/terraform-provider-azurerm#24164
`azurerm_cognitive_account_customer_managed_key` - `identity_client_id`
is no longer passed to the api when it is empty
([#24231](hashicorp/terraform-provider-azurerm#24231
`azurerm_linux_web_app_slot` - error when `service_plan_id` is identical
to the parent `service_plan_id`
([#23403](hashicorp/terraform-provider-azurerm#23403
`azurerm_management_group_template_deployment` - fixing a bug where
`template_spec_version_id` couldn&#39;t be updated
([#24072](hashicorp/terraform-provider-azurerm#24072
`azurerm_pim_active_role_assignment` - fix an importing issue by
filtering available role assignments based on the provided `scope`
([#24077](hashicorp/terraform-provider-azurerm#24077
`azurerm_pim_eligible_role_assignment` - fix an importing issue by
filtering available role assignments based on the provided `scope`
([#24077](hashicorp/terraform-provider-azurerm#24077
`azurerm_resource_group_template_deployment` - fixing a bug where
`template_spec_version_id` couldn&#39;t be updated
([#24072](hashicorp/terraform-provider-azurerm#24072
`azurerm_security_center_setting` - fix the casing for the
`setting_name` `Sentinel`
([#24210](hashicorp/terraform-provider-azurerm#24210
`azurerm_storage_account` - Fix crash when checking for
`routingInputs.PublishInternetEndpoints` and
`routingInputs.PublishMicrosoftEndpoints`
([#24228](hashicorp/terraform-provider-azurerm#24228
`azurerm_storage_share_file` - prevent panic when the file specified by
`source` is empty
([#24179](hashicorp/terraform-provider-azurerm#24179
`azurerm_subscription_template_deployment` - fixing a bug where
`template_spec_version_id` couldn&#39;t be updated
([#24072](hashicorp/terraform-provider-azurerm#24072
`azurerm_tenant_template_deployment` - fixing a bug where
`template_spec_version_id` couldn&#39;t be updated
([#24072](hashicorp/terraform-provider-azurerm#24072
`azurerm_virtual_machine` - prevent a panic by nil checking the first
element of `additional_capabilities`
([#24159](hashicorp/terraform-provider-azurerm#24159
`azurerm_windows_web_app_slot` - error when `service_plan_id` is
identical to the parent `service_plan_id`
([#23403](https://github.com/hashicorp/terraform-provider-azurerm/issues/23403))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/942/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
Copy link

github-actions bot commented May 3, 2024

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 contributions.
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 May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants