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.

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

New Resource: azurerm_bot_channel_ms_teams #4984

Merged
merged 11 commits into from
Dec 11, 2019

Conversation

HiltonGiesenow
Copy link
Contributor

I'm very new to Terraform - just dipping my toes in - but I need an MsTeams channel registration for an Azure Bot. I've tried to write it here, to contribute to the project, but I've never used Go at all, and also have no idea how to test this new resource, or how to use it in a project. So, this is a PR but also a call for help...

I've gotten Go and Make installed and gotten the project to compile, on Windows. I tried replacing the terraform-provider-azurerm.exe with the newly-generated one but that didn't work.

@katbyte katbyte added this to the v1.38.0 milestone Nov 26, 2019
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @HiltonGiesenow, so far this is on the right track. I've called out a few areas that could use a little help but overall it looks good. We also need to add docs and confirm the tests pass when you add validation to calling_web_hook

azurerm/resource_arm_bot_channel_msteams.go Outdated Show resolved Hide resolved
"calling_web_hook": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validate.NoEmptyStrings,
Copy link
Member

Choose a reason for hiding this comment

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

There is some additional validation we need here. The API only accepts https and it adds a / at the end of the URL. Unfortunately, the API accepts a url without a / at the end but always returns that url with a / at the end. This causes a permanent diff within Terraform as it tries to set a url without / and the api always returns one with /.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, but not sure how to implement this?

Copy link
Member

Choose a reason for hiding this comment

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

So you should be able to write a ValidateFunction that checks that the string starts with https and ends with /.

func validateCallingWebHook() schema.SchemaValidateFunc {
	return func(i interface{}, k string) (warnings []string, errors []error) {
		value := i.(string)
		if !strings.HasPrefix(value, "https://" ) || !strings.HasSuffix(value, "/") {
			errors = append(errors, fmt.Errorf("invalid `calling_web_hook`, must start with `https://` and end with `/`"))
		}

		return warnings, errors
	}
}

That'll need to be checked and tested as well

azurerm/resource_arm_bot_channel_msteams_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_bot_channel_msteams_test.go Outdated Show resolved Hide resolved
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
Copy link
Member

Choose a reason for hiding this comment

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

ImportStateVerifyIgnore is normally only called when a value isn't returned from the API and so we can't check whether it has been imported and set into state correctly. From my brief bit of testing, it does look like these values are returned from the api just fine. Did you see something different? If not, can we remove all the ImportStateVerifyIgnore from this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit beyond my knowledge here, but happy to make whatever changes are needed to the PR. What exactly needs to be changed?

Copy link
Member

Choose a reason for hiding this comment

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

This should be able to be removed since I saw these values being returned from the api. My concern was that it's been added so maybe you didn't see these values being returned from the api or did you copy this section in from another resource?

azurerm/resource_arm_bot_channel_msteams_test.go Outdated Show resolved Hide resolved
@@ -217,6 +217,7 @@ func Provider() terraform.ResourceProvider {
"azurerm_batch_certificate": resourceArmBatchCertificate(),
"azurerm_bot_channel_email": resourceArmBotChannelEmail(),
"azurerm_bot_channel_slack": resourceArmBotChannelSlack(),
"azurerm_bot_channel_msteams": resourceArmBotChannelMsTeams(),
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the UI has this as MSTeams but would it be more readable to split it out a little here to look like azurerm_bot_channel_ms_teams?

Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth making this microsoft_teams rather than ms so this is clearer - WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we look at sources like this
1 - https://github.com/Azure/azure-sdk-for-go/blob/7a9d2769e4a581b0b1bc609c71b59af043e05c98/services/preview/botservice/mgmt/2018-07-12/botservice/models.go
and 2 - https://docs.microsoft.com/en-us/cli/azure/bot/msteams?view=azure-cli-latest

It seems like "MsTeams" is the consistent use Microsoft themselves make - might be better if we keep it this way, especially if someone is migrating from something else?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it should be kept as MsTeams but will push again that it should be azurerm_bot_channel_ms_teams. Microsoft makes the distinction between MsTeams with capitals more often than not and we can mirror that but splitting out the words from each other

@HiltonGiesenow
Copy link
Contributor Author

Thanks guys for all the help, especially considering I'm out of my depth, so it added extra work your side! Comments above and some of the changes implemented/checked in

@ghost ghost removed the waiting-response label Nov 27, 2019
@HiltonGiesenow
Copy link
Contributor Author

@tombuildsstuff @mbfrahry hey guys, just wondering if you've had a chance to look more at this? It seems like we're very close to finishing it.

Out of interest, Matthew it sounds like you've maybe worked on deploying bots with Terraform? If so, I'm curious if you've ever managed to get an MsTeams bot to work? It requires a lot more around the appid and apppassword than a normal bot, so I'm trying to figure out the ins and outs (it's turning out there are some interesting constraints on the Service Principal that aren't obvious when you create it easily in the portal UI)

@mbfrahry
Copy link
Member

mbfrahry commented Dec 2, 2019

I haven't taken a look at implementing a MS Team Bot Service. I'd be curious to hear more about it but it might be easier to track/comment on that in a new issue/feature request?

@tombuildsstuff tombuildsstuff modified the milestones: v1.38.0, v1.39.0 Dec 3, 2019
@HiltonGiesenow
Copy link
Contributor Author

Sorry for the delay, been a hectic few days. I've made the changes - hope they're correct... As I said, I've never used Go before this, so I managed to get the "fmt" command working, and got it to build, but I still don't know how to actually test my changes. If there's still work to be done after this checkin, maybe you can give me some guidance so I can take that work off you if necessary?

@mbfrahry
Copy link
Member

mbfrahry commented Dec 9, 2019

All good. Testing this specific resource isn't very straightforward. I don't mind taking it from here as you've done the bulk of the work and testing this resource is different than testing every other resource on Azure. Do you mind if I take it off your hands and then walk you through how I'm going to test it and then how you can test it locally when I'm finished?

@HiltonGiesenow
Copy link
Contributor Author

Sounds great!

@ghost ghost added the documentation label Dec 11, 2019
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbfrahry
Copy link
Member

Thanks for the work here @HiltonGiesenow. To test this through our testing framework, you need to setup quite a few environment variables that are required to authenticate with your account.

"ARM_CLIENT_ID",
"ARM_CLIENT_SECRET",
"ARM_SUBSCRIPTION_ID",
"ARM_TENANT_ID",
"ARM_TEST_LOCATION",
"ARM_TEST_LOCATION_ALT",
"ARM_TEST_LOCATION_ALT2",

From there you can run

make testacc TESTARGS='-run=TestAccAzureRMBotChannel'

which will run through all the tests with that prefix. This will spin up and tear down actual resources on your account which could be expensive depending on how often you run them and how long they stay up but this is how we test Terraform code. Does that make sense.

Other than that, thank you again for the work here and this should make it into the next release

@mbfrahry mbfrahry changed the title Adding MsTeams Bot Channel New Resource: azurerm_bot_channel_ms_teams Dec 11, 2019
@mbfrahry mbfrahry merged commit a3e1eac into hashicorp:master Dec 11, 2019
mbfrahry added a commit that referenced this pull request Dec 11, 2019
@HiltonGiesenow
Copy link
Contributor Author

glad to have been of help, hopefully the first of many. I'll have to learn Go soon I guess ;>.

On testing - the other stuff I can figure out, but what ClientId/ClientSecret am I using? What I mean is, I assume I need to create an App, but what setup does it need exactly?

@mbfrahry
Copy link
Member

Have you seen our guide for how to setup a service principal and client secret? https://www.terraform.io/docs/providers/azurerm/guides/service_principal_client_secret.html. That might be a good place to start if you haven't.

@HiltonGiesenow
Copy link
Contributor Author

ah, no, hadn't seen that - I'm just running interactively so far - thanks so much, will give it a go!

@HiltonGiesenow HiltonGiesenow deleted the AddingMsTeamsBotChannel branch December 11, 2019 20:37
@ghost
Copy link

ghost commented Dec 16, 2019

This has been released in version 1.39.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.39.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jan 11, 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!

@ghost ghost locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants