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

Add new resource & data source: azurerm_powerbi_embedded #5152

Merged
merged 20 commits into from
Mar 17, 2020

Conversation

neil-yechenwei
Copy link
Contributor

@neil-yechenwei neil-yechenwei commented Dec 13, 2019

This PR is the code implement of the issue #5188

(fixes #5188)

@ghost ghost added dependencies size/XXL and removed size/XL labels Dec 13, 2019
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.

hi @neil-yechenwei

Thanks for this PR - I've taken a look through and left some comments inline but it appears this wants renaming to be clearer as to what it actually provisions?

Thanks!

Create: schema.DefaultTimeout(30 * time.Minute),
Read: schema.DefaultTimeout(5 * time.Minute),
Update: schema.DefaultTimeout(30 * time.Minute),
Delete: schema.DefaultTimeout(30 * time.Minute),
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these timeouts based on?

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Dec 17, 2019

Choose a reason for hiding this comment

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

Actually, azure sdk and related document doesn't explicitly point out the timeout. So I just pasted code here from other resources. Before I tried to remove this block and use the default timeout, but I failed on the CI check and threw below errors so that I have to use them here.
--- FAIL: TestResourcesSupportCustomTimeouts (0.01s)
245 --- FAIL: TestResourcesSupportCustomTimeouts/Resource/azurerm_powerbi_dedicated_capacity (0.00s)
246 provider_test.go:86: [DEBUG] Testing Resource "azurerm_powerbi_dedicated_capacity"..
247 provider_test.go:89: Resource "azurerm_powerbi_dedicated_capacity" has no timeouts block defined!


## Import

PowerBIDedicated Capacities can be imported using the `resource id`, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PowerBIDedicated Capacities can be imported using the `resource id`, e.g.
PowerBI Dedicated instances can be imported using the `resource id`, e.g.

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Dec 17, 2019

Choose a reason for hiding this comment

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

Same as above reason


The following attributes are exported:

* `id` - The ID of the PowerBIDedicated Capacity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `id` - The ID of the PowerBIDedicated Capacity.
* `id` - The ID of the PowerBI Dedicated instance.

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Dec 17, 2019

Choose a reason for hiding this comment

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

Same as above reason

Tags: tags.Expand(t),
}

future, err := client.Update(ctx, resourceGroup, name, parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

since there's a separate update function presumably this supports delta updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think you should be correct since it can update property "sku", "administrators" and "tags" successfully without attribute "ForceNew".

@neil-yechenwei neil-yechenwei changed the title Add new resource & data source: azurerm_powerbidedicated_capacity Add new resource & data source: azurerm_powerbi_dedicated_capacity Dec 17, 2019
@neil-yechenwei
Copy link
Contributor Author

Hi @tombuildsstuff , Thanks for your comments. I've updated code per your comments. For your concern, I am glad to introduce this resource here. This resource is provisioning capacity which is the feature of PowerBI Dedicated. The capacity is the container to load power bi report and dataset and also can be referenced by other apps.
The affected package is "Microsoft.PowerBIDedicated".
The affected resource is "azurerm_powerbi_dedicated_capacity"
See more details from below links:
Overview of Power BI Dedicated: https://docs.microsoft.com/en-us/azure/power-bi-embedded/
Rest api of Capacity: https://docs.microsoft.com/en-us/rest/api/power-bi-embedded/capacities/create
azure-rest-api-spec repo: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/powerbidedicated/resource-manager/Microsoft.PowerBIdedicated/stable/2017-10-01/powerbidedicated.json

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.

Thank for the revisions @neil-yechenwei, overall this is looking good but i have some questions around the naming. i'm not sure why its called a power bi dedicated capcity as a quick google shows up nothing and the portal seems to indicate it is a power BI embedded project? I think we should align with the portal and docs as that is what the user is likey familiar with rather then the API & SDK.

@@ -118,6 +119,7 @@ type Client struct {
Portal *portal.Client
Postgres *postgres.Client
PrivateDns *privatedns.Client
PowerBIDedicated *powerBIDedicated.Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this API for? looking in the portali just see "Power BI Embedded" and no mention of dedicated? Maybe this just wants to be

Suggested change
PowerBIDedicated *powerBIDedicated.Client
PowerBI *powerBIDedicated.Client

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Mar 4, 2020

Choose a reason for hiding this comment

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

This Capacity API would create the PowerBI Embedded resource which is actually a capacity as container to load the power bi report and dataset. Seems service owner wants to distinguish between powerbi capacity and powerbi workspace from api perspective. But they all are associated with PowerBI. So I think your answer is reasonable. We should align with the portal and docs as that is what the user is likey familiar with rather then the API & SDK. So I will update name from PowerBIDedicated to PowerBI. Does it make sense?

Below is my summary:

  1. azure docs:
    power bi embedded -> capacity (https://docs.microsoft.com/en-us/power-bi/developer/azure-pbie-create-capacity)
    power bi workspace (Premium) -> workspace (https://docs.microsoft.com/en-us/power-bi/service-create-the-new-workspaces)

  2. azure api spec:
    PowerBI Dedicated -> capacity (https://github.com/Azure/azure-rest-api-specs/blob/master/specification/powerbidedicated/resource-manager/Microsoft.PowerBIdedicated/stable/2017-10-01/powerbidedicated.json)
    PowerBI embedded -> workspace (https://github.com/Azure/azure-rest-api-specs/blob/master/specification/powerbiembedded/resource-manager/Microsoft.PowerBI/stable/2016-01-29/powerbiembedded.json)

  3. azure portal:
    PowerBI embedded -> capacity (https://ms.portal.azure.com/#create/Microsoft.PowerBIDedicated)

// SupportedResources returns the supported Resources supported by this Service
func (r Registration) SupportedResources() map[string]*schema.Resource {
return map[string]*schema.Resource{
"azurerm_powerbi_dedicated_capacity": resourceArmPowerBIDedicatedCapacity()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking there seems to be premium and embedded but not dedicated, so what should go here?

Suggested change
"azurerm_powerbi_dedicated_capacity": resourceArmPowerBIDedicatedCapacity()}
"azurerm_powerbi_????_capacity": resourceArmPowerBIDedicatedCapacity()}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also is capacity the right term, again looking at the portal:
image

it seems like this is a powerbi_embeded_project regardless of what the API is calling it?

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Mar 4, 2020

Choose a reason for hiding this comment

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

yes. it's the powerbi embedded project. I think we should update to "azurerm_powerbi_embedded" if we don't consider of what the API is calling. And this resource created in azure portal is called "Power BI Embedded". So my understanding is that "PowerBI Embedded" in azure doc means "PowerBI Dedicated Capacity" in azure api spec. Does it make sense?
image

Below is my understanding according by azure docs regardless of api spec:
power bi:
-> Premium (which means workspace)
-> Embedded (which means capacity)

Also refers TOC of rest api spec:
image

@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2020

After poking around some more the error i now get is:

Error: Error creating PowerBI Embedded "acctestpowerbi200305020513348527" (Resource Group "acctestRG-powerbi-200305020513348527"): powerbidedicated.CapacitiesClient#Create: Failure sending request: StatusCode=401 -- Original Error: Code="Unauthorized" Message="Unable to authorize with Azure Active Directory." Details=[{"code":"RootActivityId","message":"fc17e3fc-d438-4ec4-97db-30c5fea95984"}]

so it does seem like there is additional steps required to get this to work on a tenant/subscription that should be documented

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Mar 5, 2020

@katbyte , I found a site (https://docs.microsoft.com/en-us/power-bi/developer/register-app) which maybe can solve this issue. I cannot try it by myself since I never encountered this error. Could you try to follow the steps in this site and rerun the test cases?

@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2020

@neil-yechenwei, I tried to follow the instructions there but it still doesn't seem like power BI is enabled or accessible for our subscription:
image

@katbyte katbyte changed the title Add new resource & data source: azurerm_powerbi_dedicated_capacity Add new resource & data source: azurerm_powerbi_embedded Mar 5, 2020
@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Mar 5, 2020

@neil-yechenwei, I tried to follow the instructions there but it still doesn't seem like power BI is enabled or accessible for our subscription:
image

There is a similar issue here (https://docs.microsoft.com/en-us/power-bi/developer/embedded-troubleshoot):
image
image

Maybe you also can follow the link "sign up for power bi" in your screenshot to register.
image

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Mar 5, 2020

@katbyte , I already raised the problem to service owner. They provided below answer. So I think you just need to follow "STEP 1" in the site "https://dev.powerbi.com/apps" or just go to "app.powerbi.com" and sign-up. After login, you can rerun test case. I think it should be able to pass now. If you still have problem, please let me know.

The answer from service owner:
The reason for the error is because PBI service principal is not provisioned in your test user’s tenant.
The best thing would be if you would sign-in with your test user to PBI and hopefully this will resolve your auth issue.

@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2020

@neil-yechenwei, i did follow the link and sign up as my user.. however we use SP auth for tests an do not log in with a user. Can you ask the service owner how to authorize a given SP for the API? thanks!

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Mar 5, 2020

@katbyte , My understanding is you should be able to create capacity successfully now if you have signed in to PowerBI successfully with your user account, and your user account and your test Service Principal under the same tenant id. So could you double check whether your user account and test service principal are under the same tenant id? And could you rerun the basic test case to create the power bi embedded after your sign-in to PBI? Please let me know the test result. Thanks.

Check tenant id for your user account:
az login
az account show

Check tenant id for service principal:
az login --service-principal --username <ARM_CLIENT_ID> --password <ARM_CLIENT_SECRET> --tenant <ARM_TENANT_ID>

@ghost ghost removed the waiting-response label Mar 5, 2020
@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2020

@neil-yechenwei,

after signing in to power BI and enabling a free trial i still cannot create an embedded capacity via the portal, nor via the test SP in CI.

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Mar 5, 2020

@katbyte , may I know your email address?

@ghost ghost removed the waiting-response label Mar 5, 2020
@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2020

@neil-yechenwei, its [email protected]

@neil-yechenwei
Copy link
Contributor Author

@katbyte , I've involved you in the email.

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.

Thanks @neil-yechenwei, tests pass and this LGTM now 👍

@katbyte katbyte added this to the v2.2.0 milestone Mar 17, 2020
@katbyte katbyte merged commit baa13b0 into hashicorp:master Mar 17, 2020
katbyte added a commit that referenced this pull request Mar 17, 2020
@ghost
Copy link

ghost commented Mar 19, 2020

This has been released in version 2.2.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 = "~> 2.2.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Mar 19, 2020
@ghost
Copy link

ghost commented Apr 17, 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 👉 [email protected]. Thanks!

1 similar comment
@ghost
Copy link

ghost commented Apr 18, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 18, 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.

Support for azurerm_powerbi_dedicated_capacity
3 participants