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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

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

ziyeqf commented Dec 16, 2024

image

@ziyeqf ziyeqf marked this pull request as ready for review December 16, 2024 01:04
@ziyeqf ziyeqf changed the title azurerm_security_center_pricing - add additional update when subplan changed azurerm_security_center_pricing - force new resource to be created when subplan changes Dec 19, 2024
@yoavfr
Copy link

yoavfr commented Jan 8, 2025

@ziyeqf - looks like this is stuck? Who needs to approve?

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants