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_subscription_pricing - reset to Free tier during deletion & add existing check #21437

Merged
merged 12 commits into from Jun 8, 2023

Conversation

ziyeqf
Copy link
Contributor

@ziyeqf ziyeqf commented Apr 17, 2023

It will be reset to Free tier since the resource could not be completely deleted. We may need to point it out in change log.

Test

❯ TF_ACC=1 go test -v ./internal/services/securitycenter -run=TestAccSecurityCenterSubscriptionPricing -timeout=600m
=== RUN   TestAccSecurityCenterSubscriptionPricing_update
--- PASS: TestAccSecurityCenterSubscriptionPricing_update (296.52s)
=== RUN   TestAccSecurityCenterSubscriptionPricing_cosmosDbs
--- PASS: TestAccSecurityCenterSubscriptionPricing_cosmosDbs (167.64s)
=== RUN   TestAccSecurityCenterSubscriptionPricing_storageAccountSubplan
--- PASS: TestAccSecurityCenterSubscriptionPricing_storageAccountSubplan (161.90s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/securitycenter        626.115s

ziyeqf added 3 commits May 4, 2023 09:50
… during deletion

Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf ziyeqf force-pushed the tengzh/securitycenter/pricing_delete branch from d02f768 to 162fcca Compare May 4, 2023 01:59
ziyeqf added 2 commits May 4, 2023 10:48
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf
Copy link
Contributor Author

ziyeqf commented May 5, 2023

Hi @tombuildsstuff, I have updated this PR to use a feature block to control whether to reset to free tier, it might be more acceptable. Could you please take another look?

@github-actions github-actions bot added size/L and removed size/S labels May 5, 2023
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.

this seems like a bugfix, as it should have always reset to free? as such we can just make the change & call out the breaking change in the changelog without the features block toggle?

this seems like a safe/reasonable exception to making a breaking change? WDYT?

@ziyeqf
Copy link
Contributor Author

ziyeqf commented May 25, 2023

this seems like a bugfix, as it should have always reset to free? as such we can just make the change & call out the breaking change in the changelog without the features block toggle?

this seems like a safe/reasonable exception to making a breaking change? WDYT?

Thanks, I agreed with that.

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.

test failures:
image

@ziyeqf
Copy link
Contributor Author

ziyeqf commented May 26, 2023

Hey @katbyte, the existing check has also been added together and the tests failed because of that. To pass the tests it need manually set these printing tier to Free on testing Subscription. Btw, do you think we should add them together?

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.

The test should be resetting it to free?

// reset pricing tier to free after all tests.
	data := acceptance.BuildTestData(t, "azurerm_security_center_subscription_pricing", "test")
	data.ResourceTestSkipCheckDestroyed(t, []acceptance.TestStep{
		{
			Config: SecurityCenterSubscriptionPricingResource{}.tier("Free", "VirtualMachines"),
		},
	})

sounds like that may be broken and needing fixing?

Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf
Copy link
Contributor Author

ziyeqf commented Jun 2, 2023

I'm still working on this, please pending review.
The tests are passing now, after running optimized tests which have reset these pricing tier to free.
Screenshot 2023-06-05 at 16 14 55

Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
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.

Looks good now! thanks

LGTM 🌩️

@ziyeqf
Copy link
Contributor Author

ziyeqf commented Jun 8, 2023

kindly ping for this PR has not been merged.

@katbyte katbyte merged commit 81002fb into hashicorp:main Jun 8, 2023
14 checks passed
@github-actions github-actions bot added this to the v3.60.0 milestone Jun 8, 2023
katbyte added a commit that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants