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.

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
merged 10 commits into from
Oct 13, 2018
Merged

New Resource: azurerm_security_center_contact #2045

merged 10 commits into from
Oct 13, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Oct 9, 2018

No description provided.

@katbyte katbyte added this to the 1.17.0 milestone Oct 9, 2018
@katbyte katbyte requested a review from tombuildsstuff October 9, 2018 22:35
@ghost ghost added the size/XXL label Oct 9, 2018
@katbyte katbyte force-pushed the r-security-contact branch from 012bd4a to 4e33d2f Compare October 9, 2018 22:54
@katbyte
Copy link
Collaborator Author

katbyte commented Oct 9, 2018

#2043 should be merged first

tombuildsstuff
tombuildsstuff suggested changes Oct 10, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @katbyte

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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", "[email protected]"),
resource.TestCheckResourceAttr(resourceName, "phone", "+1-555-678-6789"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

@katbyte
Copy link
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 katbyte mentioned this pull request Oct 13, 2018
Merged
tombuildsstuff
tombuildsstuff approved these changes Oct 13, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

securitycenter -> security_center

website/azurerm.erb
<a href="/docs/providers/azurerm/r/security_center_contact.html">azurerm_security_center_contact</a>
</li>
</ul>
</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor it'd be good to sort these alphabetically

@katbyte katbyte merged commit de08599 into master Oct 13, 2018
@katbyte katbyte deleted the r-security-contact branch October 13, 2018 03:54
@katbyte katbyte changed the title New Resource: azurerm_securitycenter_contact New Resource: azurerm_security_center_contact Oct 13, 2018
katbyte added a commit that referenced this pull request Oct 13, 2018
@katbyte
Update CHANGELOG.md to include #2045
43e9c50
@ghost
Copy link

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 ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees
No one assigned
Labels
new-resource service/security-center size/XXL
Projects
None yet
Milestone
v1.17.0
Development

Successfully merging this pull request may close these issues.
2 participants
@katbyte @tombuildsstuff