-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
security_center_subscription_pricing_resource: add extensions support #22643
security_center_subscription_pricing_resource: add extensions support #22643
Conversation
8bc18f1
to
f736035
Compare
Need to solve │ * resource azurerm_security_center_subscription_pricing: extensions: Elem must be set for lists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @Ihab-Zhaika - could we include a test for this new property to ensure that it works as expected? Thanks!
internal/services/securitycenter/security_center_subscription_pricing_resource.go
Outdated
Show resolved
Hide resolved
f736035
to
09f43fd
Compare
3164755
to
b586332
Compare
b586332
to
cfdce75
Compare
b393570
to
82d84e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Ihab-Zhaika, thanks for taking the time to work on this! I left a few comments/suggestions inline but once those are addressed I can take another look at this. Thanks!
internal/services/securitycenter/security_center_subscription_pricing_resource.go
Outdated
Show resolved
Hide resolved
internal/services/securitycenter/security_center_subscription_pricing_resource.go
Outdated
Show resolved
Hide resolved
internal/services/securitycenter/security_center_subscription_pricing_resource.go
Outdated
Show resolved
Hide resolved
internal/services/securitycenter/security_center_subscription_pricing_resource_test.go
Show resolved
Hide resolved
website/docs/r/security_center_subscription_pricing.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/security_center_subscription_pricing.html.markdown
Outdated
Show resolved
Hide resolved
internal/services/securitycenter/security_center_subscription_pricing_resource.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Ihab Zhaika <ihabzhaika@microsoft.com>
@tombuildsstuff All changes are done, can you please check if it can be approved ? |
Signed-off-by: Ihab Zhaika <ihabzhaika@microsoft.com>
@catriona-m can you please run the final checks, we are on rush |
@catriona-m can you please run once again the checks |
@catriona-m Thanks for running the checks, all passed, can it be merged ? |
Hi @Ihab-Zhaika thanks for making the additional changes to this. I ran the test locally and found that it is currently failing with this error:
If you are happy for me to push changes to the PR, I can take a look at fixing this tomorrow and adding the additional test steps we need. Thanks! |
@catriona-m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good
@catriona-m Thanks for adding those improvements, when it can be merged ? |
@Ihab-Zhaika I am waiting on the tests completing, then we can request another review and hopefully this should be good to merge soon 👍 |
Sure, @catriona-m I see all tests passed |
Apologies @Ihab-Zhaika , I meant the acceptance tests. Going to merge this now! Thanks! |
I want to discuss something, Is there a slack or something or just via comments ? This PR is already completed so not for it but for next PR: in the extensions, you mentioned that if the "extension" appear then it is enabled and if not exist in the template then it is off, this is "required" behavior or something that I can change in the upcoming PRS ? Since for example we want that if the customer did not specify any extension that by default turn all of them for him, which conflicts with the approach of specifying the required extensions one by one tf apply without changing the template and the second apply would do some changes ? |
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. |
This PR contains changes to the azurerm_security_center_subscription_pricing resource which adds the extension block to the resource schema, to support for Defender for Cloud Pricing Advanced configuration.
Closes: #16217