Skip to content

azurerm_security_center_pricing - force new resource to be created when subplan changes#27805

Merged
stephybun merged 25 commits intohashicorp:mainfrom
ziyeqf:issue/security_center_princing
Feb 26, 2025
Merged

azurerm_security_center_pricing - force new resource to be created when subplan changes#27805
stephybun merged 25 commits intohashicorp:mainfrom
ziyeqf:issue/security_center_princing

Conversation

@ziyeqf
Copy link
Collaborator

@ziyeqf ziyeqf commented Oct 29, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

When subplan changes, there might be extensions enabled by default, force a new resource to be created to keep the resource consistent with configuration.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
image

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #27338

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@ziyeqf ziyeqf marked this pull request as ready for review October 30, 2024 05:53
@ziyeqf ziyeqf requested review from a team and katbyte as code owners October 30, 2024 05:53
@rcskosir rcskosir added the bug label Oct 30, 2024
@yoavfr

This comment was marked as off-topic.

@ziyeqf
Copy link
Collaborator Author

ziyeqf commented Dec 11, 2024

kindly bubble this up

@ziyeqf
Copy link
Collaborator Author

ziyeqf commented Dec 12, 2024

convert to draft to update

@WodansSon WodansSon marked this pull request as draft December 12, 2024 07:46
@yoavfr
Copy link

yoavfr commented Jan 16, 2025

@ziyeqf ping. There is a customer waiting for this since October. Microsoft is not looking very good here.

func resourceSecurityCenterSubscriptionPricing() *pluginsdk.Resource {
return &pluginsdk.Resource{
Create: resourceSecurityCenterSubscriptionPricingUpdate,
res := &pluginsdk.Resource{
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this variable declaration since we're not patching anything in the resource here

func resourceSecurityCenterSubscriptionPricingUpdate(d *pluginsdk.ResourceData, meta interface{}) error {
func resourceSecurityCenterSubscriptionPricingCreate(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).SecurityCenter.PricingClient

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change


subscriptionId := meta.(*clients.Client).Account.SubscriptionId
ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change


func resourceSecurityCenterSubscriptionPricingUpdate(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).SecurityCenter.PricingClient

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change


updateResponse, updateErr := client.Update(ctx, *id, update)
if updateErr != nil {
return fmt.Errorf("setting %s: %+v", id, updateErr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("setting %s: %+v", id, updateErr)
return fmt.Errorf("updating %s: %+v", id, updateErr)

return fmt.Errorf("setting %s: %+v", id, updateErr)
}

// The extensions list from backend might vary after `tier` changed, thus we need to retire it again.
Copy link
Member

Choose a reason for hiding this comment

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

should this be?

Suggested change
// The extensions list from backend might vary after `tier` changed, thus we need to retire it again.
// The extensions list from backend might vary after `tier` changed, thus we need to retrieve it again.

update.Properties.Extensions = extensions
_, updateErr := client.Update(ctx, *id, update)
if updateErr != nil {
return fmt.Errorf("setting %s: %+v", id, updateErr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("setting %s: %+v", id, updateErr)
return fmt.Errorf("updating %s: %+v", id, updateErr)

requiredAdditionalUpdate = currentlyFreeTier
}

updateResponse, updateErr := client.Update(ctx, *id, update)
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to reuse err here

Suggested change
updateResponse, updateErr := client.Update(ctx, *id, update)
updateResponse, err := client.Update(ctx, *id, update)

return fmt.Errorf("setting %s: %+v", id, updateErr)
}
update.Properties.Extensions = extensions
_, updateErr := client.Update(ctx, *id, update)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_, updateErr := client.Update(ctx, *id, update)
_, err = client.Update(ctx, *id, update)

Comment on lines +170 to +173
### `azurerm_security_center_princing_tier`

* Changing the property `subplan` now forces a new resource to be created.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't a 5.0 change anymore so this should be removed

@ziyeqf
Copy link
Collaborator Author

ziyeqf commented Feb 19, 2025

kindly bubble this up

@ziyeqf
Copy link
Collaborator Author

ziyeqf commented Feb 24, 2025

kindly ping @stephybun

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 @ziyeqf LGTM 👍

@stephybun stephybun merged commit 7a15269 into hashicorp:main Feb 26, 2025
33 checks passed
@github-actions github-actions bot added this to the v4.21.0 milestone Feb 26, 2025
stephybun added a commit that referenced this pull request Feb 26, 2025
@ziyeqf ziyeqf deleted the issue/security_center_princing branch February 27, 2025 02:12
jackofallops added a commit that referenced this pull request Feb 27, 2025
* Update CHANGELOG.md for #28840

* Update CHANGELOG.md #28808

* Update CHANGELOG.md #27962

* Update CHANGELOG.md for #28859

* Update for #28825

* Update CHANGELOG.md for #28864

* Update CHANGELOG.md #28539

* Update CHANGELOG.md #28685

* Update CHANGELOG.md for #28818

* Update for #28857 #28799 #28856

* Update for #28122

* Update for #28248 #27805

* Update for #28853

* Update for #28316 #28494 #28696

* Update for #28754

* Update CHANGELOG.md #28771

* Update CHANGELOG.md #28842

* Update for #28879

* Update for #28199

* Update CHANGELOG.md #28862

* prep for release v4.21.0

---------

Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com>
Co-authored-by: Wodans Son <20408400+WodansSon@users.noreply.github.com>
Co-authored-by: stephybun <steph@hashicorp.com>
Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com>
Co-authored-by: Matthew Frahry <mbfrahry@gmail.com>
Co-authored-by: Wyatt Fry <wyattfry@gmail.com>
@github-actions
Copy link
Contributor

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 Mar 29, 2025
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.

azurerm_security_center_subscription_pricing auto-enables OnUploadMalwareScanning extension with DefenderForStorageV2 subplan

4 participants