[WIP] provider/azurerm: create app_gateway resource #14426
Conversation
added hash method for ssl certs
|
@brandontosch Many thanks for sharing this pull request. I can offer to test it next week. Do you need support with anything open (e.g. tests or documentation)? As a remark, in my opinion it would be the better approach to take over from another existing pull request to "import the work" from #10413 by @pmcatominey by merging the branch of #10413 into your branch. This would keep the original commits by @pmcatominey since those do include a significant amount of code. |
There was a problem hiding this comment.
👋 hey @brandontosch
Thanks for this PR :)
I've taken a look and left some comments inline, however this is off to a good start. Most of the comments fall into one of the following categories:
- Allowing different casing on variables in the Schema, by setting
trueon thevalidation.StringInSlicemethod - Ensuring different casings are handled on the way back from Azure using a DiffSuppressFunc in the Schema
- Passing in ID's instead of Name's for Azure Resources, rather than building the ID's ourselves, since the ID's (unfortunately) aren't guaranteed to be stable in the future.
I'm hoping that makes sense? Please let me know when you want me to take another look :)
Thanks!
| "name": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validation.StringInSlice([]string{ |
There was a problem hiding this comment.
It'd be great if we can ignore the casing on this, using an DiffSuppressFunc, e.g.:
DiffSuppressFunc: ignoreCaseDiffSuppressFunc
| "tier": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validation.StringInSlice([]string{ |
There was a problem hiding this comment.
same as above, could we add a DiffSuppressFunc here?
| Elem: &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| string(network.TLSv10), |
There was a problem hiding this comment.
same as above, could we add a DiffSuppressFunc here?
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| string(network.Detection), |
There was a problem hiding this comment.
same as above, could we add a DiffSuppressFunc here?
| ValidateFunc: validation.StringInSlice([]string{ | ||
| string(network.Standard), | ||
| string(network.WAF), | ||
| }, false), |
There was a problem hiding this comment.
can we set this to true to allow different casings here?
| fqdn_list = [ | ||
| "terraform.io", | ||
| ] | ||
| } |
There was a problem hiding this comment.
can we remove the comments for the Computed fields within this test case?
| default_backend_http_settings_name = "backend-http-1" | ||
|
|
||
| path_rule { | ||
| # id = computed |
There was a problem hiding this comment.
can we remove the comments for the Computed fields here?
| } | ||
|
|
||
| backend_http_settings { | ||
| # id = computed |
There was a problem hiding this comment.
(same as above, can we remove the comments for the computed fields here)
|
|
||
| authentication_certificate { | ||
| name = "auth-1" | ||
| data = "${file("resource_arm_app_gateway_test.cer")}" |
There was a problem hiding this comment.
would it make sense to inline this value?
| ssl_certificate { | ||
| # id = computed | ||
| name = "ssl-1" | ||
| data = "${file("resource_arm_app_gateway_test.pfx")}" |
There was a problem hiding this comment.
would it make sense to inline this value?
There was a problem hiding this comment.
Thought I could get it to work, but trying to inline the pfx file contents is giving me fits. Any ideas? Was able to do the cer file no problem since the contents are just simple characters.
|
ping @brandontosch |
|
Sorry for dropping off, been a little chaotic lately. Will try and jump back in on this soon. |
removed extraneous minItems err typo fix
|
hey Brandon - this is awesome ... working to configure my own env set up and wondering if there's some way I'd be able to help you? |
|
Hi @brandontosch ... I've got an environment configured with your branch and am trying this out. Just touching base to check in on whether you are actively working on the feedback above as well? |
|
Hi @isaacsgi, I've addressed some of the requested changes (anything I gave a thumbs up to should be done), but I'd love any help you can offer. Been hard to find time lately.. |
|
Hi - moved this over to the new Provider code structure for Terraform 0.10 - see here: |
|
Closing this PR as it's been migrated to the new repo: hashicorp/terraform-provider-azurerm#120 |
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Continuing the work of @pmcatominey for #8670
Refactored to build with latest master
Still need to