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

azurerm_cost_anomaly_alert - Support for scoping alert to a specific subscription_id #24258

Merged

Conversation

daniel-edwards-nz
Copy link
Contributor

Fixes #23673

Added new required parameter subscription_id to the azurerm_cost_anomaly_alert resource which allows creation of alerts on subscriptions other than the one specified in the provider config.

I'd initially hoped to make this an optional parameter and use the provider's subscription as a default but I ran in to trouble getting the read function to work appropriately under those circumstances. As such, anyone using this already would need to update their config to include the subscription_id.

I'm somewhat new to Go and this is my first contribution here so all feedback is appreciated :-)

Test results:

=== RUN   TestAccResourceAnomalyAlert_update
=== PAUSE TestAccResourceAnomalyAlert_update
=== CONT  TestAccResourceAnomalyAlert_update
--- PASS: TestAccResourceAnomalyAlert_update (124.11s)
PASS
ok      [github.com/hashicorp/terraform-provider-azurerm/internal/services/costmanagement](http://github.com/hashicorp/terraform-provider-azurerm/internal/services/costmanagement)        127.855s

=== RUN   TestAccResourceAnomalyAlert_requiresImport
=== PAUSE TestAccResourceAnomalyAlert_requiresImport
=== CONT  TestAccResourceAnomalyAlert_requiresImport
--- PASS: TestAccResourceAnomalyAlert_requiresImport (55.27s)
PASS
ok      [github.com/hashicorp/terraform-provider-azurerm/internal/services/costmanagement](http://github.com/hashicorp/terraform-provider-azurerm/internal/services/costmanagement)        58.866s

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the pr @daniel-edwards-nz - this PR as is would be a breaking change, something we cannot do till a major version.

You should be able to make it optional and the only include the scope property in the payload if it is set and not empty?

internal/services/costmanagement/anomaly_alert_resource.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @daniel-edwards-nz - looking better but you are correct about the read - what i think can be done instead is make the property computed and then you don't have to try and conditionally set it in read

Co-authored-by: kt <kt@katbyte.me>
@daniel-edwards-nz
Copy link
Contributor Author

Thanks for the suggestions @katbyte. I've added those changes and re-run the tests and it appears to be all functioning correctly now. Let me know if you spot anything else needed :-)

@daniel-edwards-nz
Copy link
Contributor Author

Hi @katbyte, sorry for the extra ping, just wondering if you might be able to review this again please? If there are any further changes needed before this can be merged please let me know.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @daniel-edwards-nz - LGTM 🍵

@katbyte katbyte merged commit d1ebe0a into hashicorp:main Feb 16, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.92.0 milestone Feb 16, 2024
katbyte added a commit that referenced this pull request Feb 16, 2024
@daniel-edwards-nz daniel-edwards-nz deleted the cost-anomaly-subscription-scope branch February 16, 2024 03:08
dduportal added a commit to jenkins-infra/azure that referenced this pull request Feb 19, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.91.0&#34; to &#34;3.92.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.92.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.92.0&#xA;FEATURES:&#xA;&#xA;*
**New Data Source**: `azurerm_virtual_desktop_application_group`
([#24771](https://github.com/hashicorp/terraform-provider-azurerm/issues/24771))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
provider: support for the feature flag
`postgresql_flexible_server.restart_server_on_configuration_value_change
property`
([#23811](hashicorp/terraform-provider-azurerm#23811
dependencies: updating to v0.20240214.1142753 of
`github.com/hashicorp/go-azure-sdk`
([#24889](hashicorp/terraform-provider-azurerm#24889
`automation`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24858](hashicorp/terraform-provider-azurerm#24858
`maintenance`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24819](hashicorp/terraform-provider-azurerm#24819
`containerapps`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24862](hashicorp/terraform-provider-azurerm#24862
`containerservices`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24872](hashicorp/terraform-provider-azurerm#24872
`timeseriesinsights`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24889](hashicorp/terraform-provider-azurerm#24889
`azurerm_container_app_environment`: support for the
`infrastructure_resource_group_name` property
([#24361](hashicorp/terraform-provider-azurerm#24361
`azurerm_cost_anomaly_alert` - support for the `subscription_id`
property
([#24258](hashicorp/terraform-provider-azurerm#24258
`azurerm_cosmosdb_account` - add default values for the
`consistency_policy` code block
([#24830](hashicorp/terraform-provider-azurerm#24830
`azurerm_dashboard_grafana` - support for the `smtp` block
([#24717](hashicorp/terraform-provider-azurerm#24717
`azurerm_key_vault_certificates` - support for the `tags` property
([#24857](hashicorp/terraform-provider-azurerm#24857
`azurerm_key_vault_secrets` - support for the `tags` property
([#24857](hashicorp/terraform-provider-azurerm#24857
`azurerm_orchestrated_virtual_machine_scale_set` - support for the
`additional_unattend_content` block
([#24292](hashicorp/terraform-provider-azurerm#24292
`azurerm_virtual_desktop_host_pool` - support for the `vm_template`
property
([#24369](https://github.com/hashicorp/terraform-provider-azurerm/issues/24369))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_container_app_environment`: avoid unwanted
changes when updating and using `log_analytics_workspace_id`
([#24303](hashicorp/terraform-provider-azurerm#24303
`azurerm_cosmosdb_account` - fixed regression in the `backup` code block
([#24830](hashicorp/terraform-provider-azurerm#24830
`azurerm_data_factory` - allow the `git_url` property to be blank/empty
([#24879](hashicorp/terraform-provider-azurerm#24879
`azurerm_linux_web_app_slot` - the `worker_count` property now works
correctly in the `site_config` block
([#24515](hashicorp/terraform-provider-azurerm#24515
`azurerm_linux_web_app` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_linux_web_app_slot` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_private_endpoint` - fixing an issue where updating the Private
Endpoint would remove any Application Security Group Association
([#24846](hashicorp/terraform-provider-azurerm#24846
`azurerm_search_service` - fixed the update function to adjust for
changed API behaviour
([#24837](hashicorp/terraform-provider-azurerm#24837
`azurerm_search_service` - fixed the update function to adjust for
changed API behaviour
([#24903](hashicorp/terraform-provider-azurerm#24903
`azurerm_windows_web_app` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_windows_web_app_slot` - support `off` for the
`file_system_level` property
([#24877](https://github.com/hashicorp/terraform-provider-azurerm/issues/24877))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/3/">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>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
@williamarobinson
Copy link

This is great! Do you feel that this code could be used for additional resources? There are some other resources that would benefit from the subscription ID being a possible variable.
azurerm_monitor_activity_log_alert
azurerm_key_vault_secret (when going across subscriptions)

@daniel-edwards-nz
Copy link
Contributor Author

@williamarobinson, I'm glad this will be helpful :-)

While I think it could be possible to apply this same methodology to other resources my personal opinion (I'm not a project maintainer though so I can only speak for myself, not the project) is that in most cases that would be a bad idea and it would seem to go against the principles of the provider. In general the subscription is configured in one place only, in the provider config, which makes it consistent across all resources and easy to understand and use. There are obviously places where that can create frustrations though, particularly for things that cross subscription boundaries or are desirable to set up in a multi-subscription platform such as these cost alerts.

One of the main reasons I felt it was appropriate in this case is that these alerts can only be created at a subscription scope - not resource group or management group etc. This means there is very little room for it to be misunderstood or misused if the variable is added here. The value can also be automatically computed quite easily if it isn't supplied (eg. get the current provider configured subscription). To add a subscription variable for most other resources would likely lead to confusion and difficulty to set up roles and permissions etc too. The existing mechanism of a second provider configured with another subscription and used as an alias should cover the majority of use-cases.

However I think that this should be able to be implemented on a case-by-case basis for specific resources where it makes sense to do so. Particularly for those that may operate outside the traditional scope of a subscription but still need to interact with a subscription somehow. I'd be keen to explore other areas where it might be useful :-)

rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 21, 2024
…c subscription_id (hashicorp#24258)

* Add subscription_id scope to cost anomaly alert

* Update formatting

* Cleanup update logic

* Set subscription_id to optional

* Apply suggestions from code review

Co-authored-by: kt <kt@katbyte.me>

* Tidy up test

---------

Co-authored-by: kt <kt@katbyte.me>
rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 21, 2024
rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 29, 2024
…c subscription_id (hashicorp#24258)

* Add subscription_id scope to cost anomaly alert

* Update formatting

* Cleanup update logic

* Set subscription_id to optional

* Apply suggestions from code review

Co-authored-by: kt <kt@katbyte.me>

* Tidy up test

---------

Co-authored-by: kt <kt@katbyte.me>
rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 29, 2024
Copy link

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 Apr 22, 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.

Support for [thing]
3 participants