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

Merged
merged 2 commits into from
Oct 1, 2018
Merged

New Resource: azurerm_cognitive_account #962

merged 2 commits into from
Oct 1, 2018

Conversation

Nepomuceno
Copy link
Contributor

@Nepomuceno Nepomuceno commented Mar 11, 2018

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

To Do:

  • Resource Implementation:
    • Create
    • Update
    • Read
    • Delete
    • Verify the Schema design is correct
    • Add Validation
  • Tests
    • Basic Tests
    • Validation Tests
  • Documentation

Fixes #908

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 @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!

Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validateCognitiveAccountKind,
Copy link
Contributor

Choose a reason for hiding this comment

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

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),

},

"sku": {
Type: schema.TypeSet,
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 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}
}

},
},
},
Set: resourceArmCognitiveAccountSkuHash,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Type: schema.TypeString,
Required: true,
ForceNew: false,
ValidateFunc: validateCognitiveAccountSkuTier,
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 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

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

return err
}
*/
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 add this check back in? This is used to ensure the Cognitive Services account has been deleted

}

return fmt.Errorf("Bad: Get on appServiceEnvironmentsClient: %+v", err)
}*/
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 add this back in? This is used to ensure that the Cognitive Services account exists

azurerm/resource_arm_cognitive_account_test.go Outdated Show resolved Hide resolved
@tombuildsstuff
Copy link
Contributor

ping @Nepomuceno :)

@Nepomuceno
Copy link
Contributor Author

Sorry getting back to it today 😄

@metacpp metacpp self-requested a review August 2, 2018 23:46
@metacpp metacpp self-assigned this Aug 2, 2018
@metacpp metacpp removed this from the Future milestone Aug 2, 2018
@tombuildsstuff tombuildsstuff removed the request for review from metacpp August 7, 2018 16:40
@tombuildsstuff tombuildsstuff added this to the 1.17.0 milestone Sep 28, 2018
@ghost ghost added the size/XXL label Sep 28, 2018
@tombuildsstuff tombuildsstuff changed the title [WIP] New Resource: azurerm_cognitive_account New Resource: azurerm_cognitive_account Sep 28, 2018
@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Sep 28, 2018

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!

@tombuildsstuff
Copy link
Contributor

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

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.

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

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 💯

azurerm/provider.go Show resolved Hide resolved
@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-10-01 at 20 56 07

@tombuildsstuff tombuildsstuff merged commit f7d0257 into hashicorp:master Oct 1, 2018
tombuildsstuff added a commit that referenced this pull request Oct 1, 2018
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants