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_eventgrid_topic` #260

Merged

tombuildsstuff merged 7 commits into master from eventgrid-topic

Aug 21, 2017

Member

tombuildsstuff commented Aug 19, 2017

Note: Needs to be merged after #258 - since it's based on top of it

Adds support for EventGrid Topics which were announced recently. Given this is a new service it's currently limited to 2 of the Azure Regions (as such, I've mentioned this on the documentation page) - but expanded support will come in time.

I also attempted to add support for EventGrid Subscriptions at the same time, but they need some 🤔 around how's best to implement support for it; given they take a Scope (which can be a Subscription ID, Resource Group ID / Event Hub Namespace ID / EventGrid Topic ID) - which needs some extensive changes to the way Resource ID's are mapped. As such I've reversed that out for the moment.

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMEventGridTopic
=== RUN   TestAccAzureRMEventGridTopic_importBasic
--- PASS: TestAccAzureRMEventGridTopic_importBasic (158.39s)
=== RUN   TestAccAzureRMEventGridTopic_importBasicWithTags
--- PASS: TestAccAzureRMEventGridTopic_importBasicWithTags (158.91s)
=== RUN   TestAccAzureRMEventGridTopic_basic
--- PASS: TestAccAzureRMEventGridTopic_basic (173.57s)
=== RUN   TestAccAzureRMEventGridTopic_basicWithTags
--- PASS: TestAccAzureRMEventGridTopic_basicWithTags (162.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	653.339s

tombuildsstuff added 5 commits

August 19, 2017 09:05


          Upgrades to v10.3 of the Azure SDK for Go

a8321cb


          Vendoring the EventGrid SDK @ v10.3-beta

283d288


          Documentation examples should be Standard SKU's to work as expected

20548fe


          Combining EventHub + Service Bus into Messaging Resources

deeb083


          Adding support for EventGrid Topics

a64e2e7

tombuildsstuff added enhancement new-resource labels

tombuildsstuff requested a review from radeksimko

August 21, 2017 07:30

radeksimko reviewed

View reviewed changes

Member

radeksimko left a comment

I left you some comments there - the most important one is about ID/name and possibly sensitivity of access keys, other are mostly nitpicks.

azurerm/config.go Outdated

@@ @@ -293,6 +295,12 @@ func (c *Config) getArmClient() (*ArmClient, error) { @@
               	img.Sender = autorest.CreateSender(withRequestLogging())
               	client.imageClient = img
+              	ehtc := eventgrid.NewTopicsClientWithBaseURI(endpoint, c.SubscriptionID)

Member

radeksimko Aug 21, 2017

Nit: What does the ehtc mean here in this context? Shouldn't it be egt (Event Grid Topics) or something like that?

Member Author

tombuildsstuff Aug 21, 2017

fixed.

azurerm/provider.go

-              			"azurerm_key_vault":                   resourceArmKeyVault(),
+              			"azurerm_image":     resourceArmImage(),
+              			"azurerm_key_vault": resourceArmKeyVault(),

Member

radeksimko Aug 21, 2017

Nit: Is there any particular reason why these 2 resources deserve to be separated from others, and not azurerm_express_route_circuit?

Member Author

tombuildsstuff Aug 21, 2017

Given a lot of the resources share the same initial character, the resources are vaguely grouped by letter so it was easier to skim-read, but happy to move them up?

Member

radeksimko Aug 21, 2017

Nah, that's ok - I was just curious what's the logic behind it 🙂

azurerm/resource_arm_eventgrid_topic.go Outdated

+              func resourceArmEventGridTopicCreateUpdate(d *schema.ResourceData, meta interface{}) error {
+              	client := meta.(*ArmClient).eventGridTopicsClient
+              	log.Printf("[INFO] preparing arguments for AzureRM EventGrid Topic creation.")

Member

radeksimko Aug 21, 2017

I think this log message could be a lot more useful if it was moved down to line 71 and also displayed contents of properties before passing it to the SDK.

azurerm/resource_arm_eventgrid_topic.go

+              		return err
+              	}
+              	read, err := client.Get(resourceGroup, name)

Member

radeksimko Aug 21, 2017

Is there any reason we cannot use name as ID and avoid making that extra API call? Otherwise this is susceptible to dataloss (and dangling resource) in case the second call fails for any reason.

Member Author

tombuildsstuff Aug 21, 2017

Consistency with the Azure API's (and existing resources) - which use ID's in the format:

/subscriptions/foo/resourceGroups/bar/provider/Microsoft.Blah/eventGrid/name

There's an issue open in the SDK to expose a method of obtaining the Resource ID without making this Get Request; once that's done we can switch this out (but that's a future enhancement).

Member

radeksimko Aug 21, 2017

I see, that's sad 😢

azurerm/resource_arm_eventgrid_topic.go Outdated

+              	resp, err := client.Get(resourceGroup, name)
+              	if err != nil {
+              		if responseWasNotFound(resp.Response) {
+              			d.SetId("")

Member

radeksimko Aug 21, 2017

Nitpick: We tend to log this as WARN to make such cases easier to spot in the debug log.

Member Author

tombuildsstuff Aug 21, 2017

Fixed. (FWIW these should be visible as 404's from the SDK anyway which tends to be the easiest way to grep for them, given the debug logs are very chatty currently)

azurerm/resource_arm_eventgrid_topic.go

+              	}
+              	d.Set("primary_access_key", keys.Key1)
+              	d.Set("secondary_access_key", keys.Key2)

Member

radeksimko Aug 21, 2017

Are those access keys sensitive in any way - i.e. would it cause problem if they were exposed in the plan/apply output? If yes, then they should be also marked as Sensitive in the schema.

azurerm/resource_arm_eventgrid_topic.go Outdated

+              			return nil
+              		}
+              		return err

Member

radeksimko Aug 21, 2017

I think you can leave out this return here as the one below is equivalent given the circumstances.

azurerm/resource_arm_eventgrid_topic_test.go Outdated

+              			{
+              				Config: config,
+              				Check: resource.ComposeTestCheckFunc(
+              					testCheckAzureRMEventGridTopicExists("azurerm_eventgrid_topic.test"),

Member

radeksimko Aug 21, 2017

It would be nice to add more checks here, at least basic state checks with resource.TestCheckResourceAttr and/or resource.TestCheckResourceAttrSet.

Member Author

tombuildsstuff Aug 21, 2017

As discussed on Slack, we mostly lean on Import tests here - but I'd added checks for the optional/computed fields

tombuildsstuff added 2 commits

August 21, 2017 13:09


          Responding to feedback

cb5b911


          Adding checks for the optional/computed fields

41a22cf

Member Author

tombuildsstuff commented Aug 21, 2017

Updated, here's the result of the updated tests:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMEventGridTopic_basic
=== RUN   TestAccAzureRMEventGridTopic_basic
--- PASS: TestAccAzureRMEventGridTopic_basic (101.45s)
=== RUN   TestAccAzureRMEventGridTopic_basicWithTags
--- PASS: TestAccAzureRMEventGridTopic_basicWithTags (96.81s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	198.277s

and the import tests:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMEventGridTopic_import
=== RUN   TestAccAzureRMEventGridTopic_importBasic
--- PASS: TestAccAzureRMEventGridTopic_importBasic (108.05s)
=== RUN   TestAccAzureRMEventGridTopic_importBasicWithTags
--- PASS: TestAccAzureRMEventGridTopic_importBasicWithTags (113.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	222.029s

radeksimko approved these changes

View reviewed changes

Member

radeksimko left a comment

LGTM

tombuildsstuff merged commit 1ef420d into master

tombuildsstuff deleted the eventgrid-topic branch

August 21, 2017 11:31

tombuildsstuff added a commit that referenced this pull request


          Updating to include #260 / azurerm_eventgrid_topic

09ca411

ghost commented Apr 1, 2020

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!

hashicorp locked and limited conversation to collaborators

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