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.

Sign up for GitHub

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

Jump to bottom

New Resource: azurerm_security_center_subscription_pricing #2043

Merged

katbyte merged 6 commits into master from r-security-pricing

Oct 13, 2018

Collaborator

katbyte commented Oct 9, 2018

No description provided.


          added new resource azurerm_securitycenter_subscription_pricing

f31eaa0

katbyte added new-resource service/security-center labels

katbyte added this to the 1.17.0 milestone

katbyte requested a review from tombuildsstuff

October 9, 2018 19:31

ghost added the size/XXL label

katbyte added 2 commits

October 9, 2018 15:45


          Added docs

05d7c54


          fix small typo in docs

64dfcad

katbyte mentioned this pull request

New Resource: azurerm_security_center_contact #2045

Merged

metacpp reviewed

View reviewed changes

azurerm/config.go Show resolved Hide resolved

metacpp reviewed

View reviewed changes

website/docs/r/securitycenter_subscription_pricing.markdown Outdated


		# azurerm_securitycenter_subscription_pricing

		Manages the subscription's Security Center pricing tier.

Contributor

metacpp Oct 9, 2018

Does this resource need Owner permission here?

Contributor

tombuildsstuff Oct 10, 2018

From memory I believe this does - but it'd be worth confirming/documenting here

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              		},
+              		Schema: map[string]*schema.Schema{
+              			"tier": {

Contributor

metacpp Oct 9, 2018

Does the API support in-place update ?

Collaborator Author

katbyte Oct 10, 2018

Yep

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              	client := meta.(*ArmClient).securityCenterPricingClient
+              	ctx := meta.(*ArmClient).StopContext
+              	resp, err := client.GetSubscriptionPricing(ctx, "default")

Contributor

metacpp Oct 9, 2018

merge to single line?

Collaborator Author

katbyte Oct 10, 2018

It cannot be done as resp is used below

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              	client := meta.(*ArmClient).securityCenterPricingClient
+              	ctx := meta.(*ArmClient).StopContext
+              	resp, err := client.GetSubscriptionPricing(ctx, "default")

Contributor

metacpp Oct 9, 2018

Shall we move "default" to a constant variable?

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              	resp, err := client.GetSubscriptionPricing(ctx, "default")
+              	if err != nil {
+              		if utils.ResponseWasNotFound(resp.Response) {

Contributor

metacpp Oct 9, 2018

Is it because of terraform import ?

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              		},
+              	}
+              	_, err := client.UpdateSubscriptionPricing(ctx, "default", pricing)

Contributor

metacpp Oct 9, 2018

move to constant variable?

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              		return fmt.Errorf("Error creating/updating Security Center Subscription pricing: %+v", err)
+              	}
+              	resp, err := client.GetSubscriptionPricing(ctx, "default")

Contributor

metacpp Oct 9, 2018

Is the GetSubscriptionPricing duplicated with resourceArmSecurityCenterSubscriptionPricingRead ?

metacpp reviewed

View reviewed changes

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated Show resolved Hide resolved

metacpp approved these changes

View reviewed changes

Contributor

metacpp left a comment

LGTM with some suggestions.

tombuildsstuff approved these changes

View reviewed changes

Contributor

tombuildsstuff left a comment

a few minor comments but this otherwise LGTM 👍

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              			"tier": {
+              				Type:             schema.TypeString,
+              				Required:         true,
+              				DiffSuppressFunc: suppress.CaseDifference,

Contributor

tombuildsstuff Oct 9, 2018

we can remove this since we're requiring the case matches below

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              }
+              func resourceArmSecurityCenterSubscriptionPricingDelete(_ *schema.ResourceData, _ interface{}) error {
+              	return nil //cannot be deleted

Contributor

tombuildsstuff Oct 9, 2018

can we revert to whatever the default value is? we do this for the Postgresql/MySql configuration value resources

azurerm/resource_arm_securitycenter_subscription_pricing.go Outdated

+              	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+              )
+              func resourceArmSecurityCenterSubscriptionPricing() *schema.Resource {

Contributor

tombuildsstuff Oct 10, 2018

thinking about this longer term - I think this'd make more sense as a nested element on the azurerm_subscription resource (but this is fine as an independent resource for the moment) 🤔

azurerm/provider.go Outdated

@@ @@ -243,6 +243,7 @@ func Provider() terraform.ResourceProvider { @@
               			"azurerm_route":                                   resourceArmRoute(),
               			"azurerm_route_table":                             resourceArmRouteTable(),
               			"azurerm_search_service":                          resourceArmSearchService(),
+              			"azurerm_securitycenter_subscription_pricing":     resourceArmSecurityCenterSubscriptionPricing(),

Contributor

tombuildsstuff Oct 10, 2018

securitycenter -> security_center

azurerm/config.go Show resolved Hide resolved

website/docs/r/securitycenter_subscription_pricing.markdown Outdated

+              page_title: "Azure Resource Manager: azurerm_securitycenter_subscription_pricing"
+              sidebar_current: "docs-azurerm-securitycenter-subscription-pricing"
+              description: |-
+                  Manages the subscription's Security Center pricing tier.

Contributor

tombuildsstuff Oct 10, 2018

I think this'd read clearer as Manages the Pricing Tier for Azure Security Center in the current subscription - what do you think (and below too)

website/docs/r/securitycenter_subscription_pricing.markdown Outdated


		# azurerm_securitycenter_subscription_pricing

		Manages the subscription's Security Center pricing tier.

Contributor

tombuildsstuff Oct 10, 2018

From memory I believe this does - but it'd be worth confirming/documenting here

website/docs/r/securitycenter_subscription_pricing.markdown Outdated


		* `tier` - (Required) The pricing tier to use. Must be one of `Free` or `Standard`.

		~> NOTE: Changing the pricing tier to `Standard` affects all resources in the subscription and could be quite costly.

Contributor

tombuildsstuff Oct 10, 2018

👍

tombuildsstuff added the waiting-response label

katbyte added a commit that referenced this pull request


          Address PR #2043 comments

8fa67b0

katbyte force-pushed the r-security-pricing branch from 8fa67b0 to 0f32cf6 Compare

October 13, 2018 02:04

katbyte added a commit that referenced this pull request


          Address PR #2043 comments

0f32cf6


          Address PR #2043 comments

1ee95b2

katbyte force-pushed the r-security-pricing branch from 0f32cf6 to 1ee95b2 Compare

October 13, 2018 02:05

Collaborator Author

katbyte commented Oct 13, 2018

Tests pass:

gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMSecurityCenterSubscriptionPricing -timeout 180m
=== RUN   TestAccAzureRMSecurityCenterSubscriptionPricing_update
--- PASS: TestAccAzureRMSecurityCenterSubscriptionPricing_update (39.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	39.416s

katbyte mentioned this pull request

New Resource: azurerm_security_center_workspace #2072

Merged


          Simplified code a little

9029ed9

tombuildsstuff removed the waiting-response label

tombuildsstuff approved these changes

View reviewed changes

Contributor

tombuildsstuff left a comment

a few minor things but this otherwise LGTM 👍

azurerm/resource_arm_security_center_subscription_pricing_test.go

+              )
+              func TestAccAzureRMSecurityCenterSubscriptionPricing_update(t *testing.T) {
+              	resourceName := "azurerm_security_center_subscription_pricing.test"

Contributor

tombuildsstuff Oct 13, 2018

this is fine for now - we may want to add a feature-toggle for this in time

website/docs/r/security_center_subscription_pricing.markdown Outdated

+              The pricing tier can be imported using the `resource id`, e.g.
+              ```shell
+              terraform import azurerm_securitycenter_subscription_pricing.example /subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Security/pricings/default

Contributor

tombuildsstuff Oct 13, 2018

securitycenter -> security_center

website/docs/r/security_center_subscription_pricing.markdown Outdated


		The following arguments are supported:

		* `tier` - (Required) The pricing tier to use. Must be one of `Free` or `Standard`.

Contributor

tombuildsstuff Oct 13, 2018

to match the other resources this'd be better as:

Possible values are `Free` and `Standard`

website/docs/r/security_center_subscription_pricing.markdown Outdated


		Manages the Pricing Tier for Azure Security Center in the current subscription.

		~> NOTE: Owner access permission is required.

Contributor

tombuildsstuff Oct 13, 2018

Would this be clearer as:

This resource requires the `Owner` permission on the Subscription

tombuildsstuff changed the title ~~New Resource: azurerm_securitycenter_subscription_pricing~~ New Resource: azurerm_security_center_subscription_pricing

tombuildsstuff reviewed

View reviewed changes

website/docs/r/security_center_subscription_pricing.markdown


		# azurerm_security_center_subscription_pricing

		Manages the Pricing Tier for Azure Security Center in the current subscription.

Contributor

tombuildsstuff Oct 13, 2018

IMO we should add a note about Deletion here

katbyte added a commit that referenced this pull request


          Address more PR #2043 comments

433fac6


          Address more PR #2043 comments

4a4268b

katbyte force-pushed the r-security-pricing branch from 433fac6 to 4a4268b Compare

October 13, 2018 03:47

katbyte merged commit 434862d into master

katbyte deleted the r-security-pricing branch

October 13, 2018 03:52

katbyte added a commit that referenced this pull request


          Update CHANGELOG.md to include #2043

4b598c9

ghost commented Mar 6, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new-resource service/security-center size/XXL