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_cognitive_account` #962

Merged

tombuildsstuff merged 2 commits into hashicorp:master from Nepomuceno:cognitive-services

Oct 1, 2018

Contributor

Nepomuceno commented Mar 11, 2018 •

edited by tombuildsstuff

Loading

Note: This is my first full resource so this might take more time to implement than desired

To Do:

Fixes #908

bflad added new-resource service/cognitive-services labels

tombuildsstuff suggested changes

View reviewed changes

Contributor

tombuildsstuff left a comment

hey @Nepomuceno

Thanks for this PR - apologies for the delayed review here!

I've taken a look through and left some comments inline but this is off to a good start; if we can fix up the comments (and then add some documentation) this should be good 👍

Regarding vendoring we're trying to migrate to the latest version of the Azure SDK at the moment; so I'd suggest continuing using the version you've vendored and we can push a commit to upgrade to the latest version once we've upgraded.

Thanks!

azurerm/resource_arm_cognitive_account.go Outdated

+              				Type:         schema.TypeString,
+              				Required:     true,
+              				ForceNew:     true,
+              				ValidateFunc: validateCognitiveAccountKind,

Contributor

tombuildsstuff Mar 30, 2018

we can actually in-line these by doing:

ValidateFunc: validation.StringInSlice([]string{
	"Academic",
	"Bing.Autosuggest",
	"Bing.Autosuggest.v7",
	"Bing.CustomSearch",
	"Bing.Search",
	"Bing.Search.v7",
	"Bing.Speech",
	"Bing.SpellCheck",
	"Bing.SpellCheck.v7",
	"ComputerVision",
	"ContentModerator",
	"CustomSpeech",
	"Emotion",
	"Face",
	"LUIS",
	"Recommendations",
	"SpeakerRecognition",
	"Speech",
	"SpeechTranslation",
	"TextAnalytics"
	"TextTranslation",
	"WebLM",
}, false),

azurerm/resource_arm_cognitive_account.go Outdated

+              			},
+              			"sku": {
+              				Type:     schema.TypeSet,

Contributor

tombuildsstuff Mar 30, 2018

we should be able to make this a TypeList rather than a TypeSet- which means it can be expanded via:

inputs := d.Get("sku").([]interface{})
input := inputs[0].(map[string]interface{})
# key into input to get the values

and flattened via:

func flatten... (input *cognitiveservuces.Sku)[]interface{} {
  output := make(map[string]interface{}, 0)
  output["foo"] = input.Bar
  return []interface{}{output}
}

azurerm/resource_arm_cognitive_account.go Outdated

+              						},
+              					},
+              				},
+              				Set: resourceArmCognitiveAccountSkuHash,

Contributor

tombuildsstuff Mar 30, 2018

(which would mean we're able to remove this)

azurerm/resource_arm_cognitive_account.go Outdated

+              							Type:         schema.TypeString,
+              							Required:     true,
+              							ForceNew:     false,
+              							ValidateFunc: validateCognitiveAccountSkuTier,

Contributor

tombuildsstuff Mar 30, 2018

can we remove the validation for the SKU Name & Tier? In general it's simpler to let Azure handle that as it can vary by account/region

azurerm/resource_arm_cognitive_account.go Outdated

+              func resourceArmCognitiveAccountCreate(d *schema.ResourceData, meta interface{}) error {
+              	client := meta.(*ArmClient).cognitiveAccountsClient
+              	ctx := meta.(*ArmClient).StopContext
+              	fmt.Printf("[INFO] preparing arguments for Azure ARM  Cognitive Servies Account creation.\n")

Contributor

tombuildsstuff Mar 30, 2018

we should be able to make this:

log.Printf("[INFO] preparing arguments for Azure ARM  Cognitive Servies Account creation")

azurerm/resource_arm_cognitive_account.go Outdated Show resolved Hide resolved

azurerm/resource_arm_cognitive_account_test.go Outdated

+              				return err
+              			}
+              		*/

Contributor

tombuildsstuff Mar 30, 2018

can we add this check back in? This is used to ensure the Cognitive Services account has been deleted

azurerm/resource_arm_cognitive_account_test.go Outdated

+              			}
+              			return fmt.Errorf("Bad: Get on appServiceEnvironmentsClient: %+v", err)
+              		}*/

Contributor

tombuildsstuff Mar 30, 2018

can we add this back in? This is used to ensure that the Cognitive Services account exists

azurerm/resource_arm_cognitive_account_test.go Show resolved Hide resolved

azurerm/resource_arm_cognitive_account_test.go Outdated Show resolved Hide resolved

tombuildsstuff added the waiting-response label

tombuildsstuff added this to the Future milestone

Contributor

tombuildsstuff commented Jun 19, 2018

ping @Nepomuceno :)

Contributor Author

Nepomuceno commented Jun 20, 2018

Sorry getting back to it today 😄

metacpp self-requested a review

August 2, 2018 23:46

metacpp self-assigned this

metacpp removed this from the Future milestone

tombuildsstuff assigned tombuildsstuff and unassigned metacpp

tombuildsstuff removed the request for review from metacpp

August 7, 2018 16:40

tombuildsstuff removed the waiting-response label

tombuildsstuff added this to the 1.17.0 milestone

ghost added the size/XXL label

tombuildsstuff changed the title ~~[WIP] New Resource: azurerm_cognitive_account~~ New Resource: azurerm_cognitive_account

Contributor

tombuildsstuff commented Sep 28, 2018 •

edited

Loading

hey @Nepomuceno

Just to let you know that since this required a rebase I've gone ahead and done that (and at the same time made the pending changes needed so that we can get this merged).

Update: just noticed there's a 58.72MB binary in the commit history - rather than merging this binary in (and having that become part of the Git history that gets downloaded) I'm going to flatten these commits down (whilst retaining your authorship) - I hope you don't mind!.

Thanks!

Contributor

tombuildsstuff commented Sep 28, 2018

Tests pass:

$ acctests azurerm TestAccAzureRMCognitiveAccount_
=== RUN   TestAccAzureRMCognitiveAccount_basic
--- PASS: TestAccAzureRMCognitiveAccount_basic (96.87s)
=== RUN   TestAccAzureRMCognitiveAccount_complete
--- PASS: TestAccAzureRMCognitiveAccount_complete (93.81s)
=== RUN   TestAccAzureRMCognitiveAccount_update
--- PASS: TestAccAzureRMCognitiveAccount_update (100.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	291.778s

tombuildsstuff reviewed

View reviewed changes

azurerm/provider.go Show resolved Hide resolved

tombuildsstuff approved these changes

View reviewed changes

Contributor

tombuildsstuff left a comment

LGTM 👍 - going to ask @katbyte to take a look through too since I've pushed some of these changes

tombuildsstuff requested a review from katbyte

September 28, 2018 21:53

katbyte approved these changes

View reviewed changes

Collaborator

katbyte left a comment

LGTM 💯

azurerm/provider.go Show resolved Hide resolved

Nepomuceno and others added 2 commits

October 1, 2018 20:14


          New Resource: azurerm_cognitive_account

dcc7c6b


          Updating to v21.1.0 of the Azure SDK

14bde63

Contributor

tombuildsstuff commented Oct 1, 2018

Tests pass:

tombuildsstuff merged commit f7d0257 into hashicorp:master

tombuildsstuff added a commit that referenced this pull request


          Updating to include #962

81ab3b8

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.

Labels

new-resource service/cognitive-services size/XXL