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_contact #2045

Merged

katbyte merged 10 commits into master from r-security-contact

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 22:35

ghost added the size/XXL label

katbyte added 4 commits

October 9, 2018 15:45


          Added docs

05d7c54


          added new resource azurerm_securitycenter_contact

3d94cd9


          Added docs

4e33d2f


          fix small typo in docs

64dfcad

katbyte force-pushed the r-security-contact branch from 012bd4a to 4e33d2f Compare

October 9, 2018 22:54

Collaborator Author

katbyte commented Oct 9, 2018

#2043 should be merged first

tombuildsstuff requested changes

View reviewed changes

Member

tombuildsstuff left a comment

This mostly LGTM (aside from the missing documentation) but as discussed on Slack I'm wondering if this'd be better as a combined resource for all of the Security Center items we've got so far, with blocks per item. Whilst that'd be a little more complex of a resource - IMO it'd provide a better UX?

Thanks!

azurerm/provider.go Outdated

@@ @@ -244,6 +244,7 @@ func Provider() terraform.ResourceProvider { @@
               			"azurerm_route_table":                             resourceArmRouteTable(),
               			"azurerm_search_service":                          resourceArmSearchService(),
               			"azurerm_securitycenter_subscription_pricing":     resourceArmSecurityCenterSubscriptionPricing(),
+              			"azurerm_securitycenter_contact":                  resourceArmSecurityCenterContact(),

Member

tombuildsstuff Oct 10, 2018

(as in the other PR) I think this'd be better as security_center?

azurerm/resource_arm_securitycenter_contact.go Outdated

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

Member

tombuildsstuff Oct 10, 2018

I don't think this is necessary for a phone number?

azurerm/resource_arm_securitycenter_contact.go Outdated

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

Member

tombuildsstuff Oct 10, 2018

is there a reason to do this?

azurerm/resource_arm_securitycenter_contact.go Outdated

+              // Invalid security contact name 'default0' was provided. Expected 'default1'
+              // Message="Invalid security contact name 'default2' was provided. Expected 'default1'"
+              func resourceArmSecurityCenterContact() *schema.Resource {

Member

tombuildsstuff Oct 10, 2018

as discussed on Slack - I think it'd be worth combining these into an azurerm_security_center resource with blocks for each thing?

azurerm/resource_arm_securitycenter_contact.go Outdated

+              // Invalid security contact name 'default0' was provided. Expected 'default1'
+              // Message="Invalid security contact name 'default2' was provided. Expected 'default1'"
+              func resourceArmSecurityCenterContact() *schema.Resource {

Member

tombuildsstuff Oct 10, 2018

as discussed on Slack - I think it'd be worth combining these into an azurerm_security_center resource with blocks for each thing?

Member

tombuildsstuff Oct 10, 2018

can we document this resource too?

azurerm/resource_arm_securitycenter_contact.go Outdated

+              	ctx := meta.(*ArmClient).StopContext
+              	//id is in format of `/subscriptions/20ff7fc3-e762-44dd-bd96-b71116dcdc23/providers/Microsoft.Security/securityContacts/john`
+              	//parseAzureResourceID doesn't support id without a resource group

Member

tombuildsstuff Oct 10, 2018

we should probably think about creating an overload which doesn't require it - there's a few instances of this now (key vault)

azurerm/resource_arm_securitycenter_contact_test.go Outdated

+              				Check: resource.ComposeTestCheckFunc(
+              					testCheckAzureRMSecurityCenterContactExists(resourceName),
+              					resource.TestCheckResourceAttr(resourceName, "email", "email2@example.com"),
+              					resource.TestCheckResourceAttr(resourceName, "phone", "+1-555-678-6789"),

Member

tombuildsstuff Oct 10, 2018

could we make this an international number to ensure that works too? +44 1234 567890 should work as an example

Collaborator Author

katbyte Oct 13, 2018

Oddly enough international numbers do not seem to work, for that and some other variations i tried:

* azurerm_security_center_contact.test: Error updating Security Center Contact: security.ContactsClient#Update: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidInputJson" Message="Invalid security contact phone was provided."

tombuildsstuff added the waiting-response label

katbyte added 3 commits

October 12, 2018 19:05


          Address PR #2043 comments

1ee95b2


          Merge branch 'r-security-pricing' into r-security-contact

3eab585


          Address PR #2045 comments

30f52c5

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=TestAccAzureRMSecurityCenterContact -timeout 180m
=== RUN   TestAccAzureRMSecurityCenterContact_basic
--- PASS: TestAccAzureRMSecurityCenterContact_basic (18.29s)
=== RUN   TestAccAzureRMSecurityCenterContact_update
--- PASS: TestAccAzureRMSecurityCenterContact_update (26.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	(cached)

katbyte mentioned this pull request

New Resource: azurerm_security_center_workspace #2072

Merged


          Added checkDestroy function

61d3be3

tombuildsstuff approved these changes

View reviewed changes

Member

tombuildsstuff left a comment

a few minor comments but this otherwise LGTM 👍

azurerm/resource_arm_security_center_contact.go Outdated

+              // Invalid security contact name was provided - only 'defaultX' is allowed where X is an index
+              // Invalid security contact name 'default0' was provided. Expected 'default1'
+              // Message="Invalid security contact name 'default2' was provided. Expected 'default1'"
+              const resourceArmSecurityCenterContactName = "default1"

Member

tombuildsstuff Oct 13, 2018

minor can we ditch resourceArm at the start of this

azurerm/resource_arm_security_center_subscription_pricing.go

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

Member

tombuildsstuff Oct 13, 2018

IMO we should probably log this?

website/docs/r/security_center_contact.markdown Outdated

+              The contact can be imported using the `resource id`, e.g.
+              ```shell
+              terraform import azurerm_securitycenter_contact.example /subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Security/securityContacts/default1

Member

tombuildsstuff Oct 13, 2018

securitycenter -> security_center

website/azurerm.erb

+                              <a href="/docs/providers/azurerm/r/security_center_contact.html">azurerm_security_center_contact</a>
+                              </li>
+                            </ul>
+                          </li>

Member

tombuildsstuff Oct 13, 2018

minor it'd be good to sort these alphabetically

tombuildsstuff removed the waiting-response label


          Address more PR #2045 comments

53b08d2

katbyte merged commit de08599 into master

katbyte deleted the r-security-contact branch

October 13, 2018 03:54

katbyte changed the title ~~New Resource: azurerm_securitycenter_contact~~ New Resource: azurerm_security_center_contact

katbyte added a commit that referenced this pull request


          Update CHANGELOG.md to include #2045

43e9c50

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 👉 hashibot-feedback@hashicorp.com. 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.