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_security_center_pricing - force new resource to be created when subplan changes #27805

Merged
merged 25 commits into from
Feb 26, 2025

Conversation

ziyeqf
Copy link
Contributor

@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 katbyte and a team 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
Contributor Author

ziyeqf commented Dec 11, 2024

kindly bubble this up

@ziyeqf
Copy link
Contributor 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

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
Contributor Author

ziyeqf commented Feb 19, 2025

kindly bubble this up

@ziyeqf
Copy link
Contributor 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 <[email protected]>
Co-authored-by: Wodans Son <[email protected]>
Co-authored-by: stephybun <[email protected]>
Co-authored-by: catriona-m <[email protected]>
Co-authored-by: Matthew Frahry <[email protected]>
Co-authored-by: Wyatt Fry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants