migrate azurerm_synapse_role_assignment to support new roles and scopes

[njuCZ]

fix #10141

in old api version, the only supported role is Workspace Admin, Sql Admin and Apache Spark Admin. The scope is workspace.

in new api version, exsiting roles are renamed and new roles are added, Users could also specify different scope: workspace, spark pool or others (not suported in terraform, so not added in this PR) .

[njuCZ]

this PR contains breaking changes, is it possible to be merged in the next major version 3.0 ?

[katbyte]

@njuCZ - is there a reason we can't map the old roles to the new ones so this isn't a breaking change? (at least for upgrading?)

[njuCZ]

@katbyte we could map the old roles to the new, in this PR I also added a stateUpgrade func to make it. But in the old api, the scope of role assignment is always workspace level, while the new api could be workspace level, sparkpool level and others. So I removed property synapse_workspace_id and add property synapse_scope, I think this is a breaking change for users.

[chrsundermann]

Does it make sense not to hardcode possible roles? As soon as new roles will be integrated they can't be picked and code has to be updated.

To update on this:
The implemented synapse_scope want the id of the synapse workspace but there are more settings possible. If you request possible values via az cli you get this output:

[katbyte]

But in the old api, the scope of role assignment is always workspace level, while the new api could be workspace level, sparkpool level and others

if we default to workspace level and mark that as optional it's not a breaking change?

[njuCZ]

@katbyte yes, in the state upgrade func I did that. I thought changing the schema name (synapse_workspace_id -> syanpse_scope) belongs to breaking change because we need users to modify the property name in TF config file. If this change is OK, I think we could remove the breaking change label.

[njuCZ]

another way to make this PR: we could mark synapse_workspace_id optional, and add other schema for scope, for example: spark_pool_id. In this way, we would have multiple specific schema to represent scope and only one of them should be specified.

[katbyte]

another way to make this PR: we could mark synapse_workspace_id optional, and add other schema for scope, for example: spark_pool_id. In this way, we would have multiple specific schema to represent scope and only one of them should be specified.

I like this id! and then we get proper validation on the id of the scope and no breaking change, this works for me!

[njuCZ]

@katbyte I have refined this PR, now there is no breaking change. All acctest could pass.

[njuCZ]

@chrsundermann sorry for late reply. Currently there is not many roles, so we could hardcode in the validation function and provide more friendly user experience . If there are many roles added in future, we could add another data source for role definition.

As for synapse scope, since integrationRuntimes, credentials, linkedservice are not supported in terraform yet, so in this PR, I don't support them. I think it's easy to add support for those scope in future.

[katbyte]

@njuCZ - looks like all the tests are now filing:

------- Stdout: -------
=== RUN   TestAccSynapseRoleAssignment_basic
=== PAUSE TestAccSynapseRoleAssignment_basic
=== CONT  TestAccSynapseRoleAssignment_basic
    testing.go:620: Step 1/2 error: Error running apply: exit status 1
        
        Error: waiting on creation for Synapse Workspace "acctestsw210610181822439671" (Resource Group "acctestRG-synapse-210610181822439671"): Code="CreateWorkspaceError" Message="An error has occured while creating the workspace. Correlation Id: 9e48fcab-9fa9-4f04-80e5-3538b340c54f"
        
          with azurerm_synapse_workspace.test,
          on terraform_plugin_test.tf line 26, in resource "azurerm_synapse_workspace" "test":
          26: resource "azurerm_synapse_workspace" "test" {
        
--- FAIL: TestAccSynapseRoleAssignment_basic (881.76s)
FAIL

------- Stderr: -------
2021/06/10 18:18:22 [DEBUG] not using binary driver name, it's no longer needed
2021/06/10 18:18:22 [DEBUG] not using binary driver name, it's no longer needed

[njuCZ]

@katbyte the acctest in teamcity could pass now.

[katbyte]

one last comment i think

[njuCZ]

@katbyte thanks for your suggestions. I have added support for old roles and add a suppressFunc to avoid diff between old roles and new roles.

[katbyte]

Thanks @njuCZ - LGTM aside from one comment which i'll fix now 🍰

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.