-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
migrate azurerm_synapse_role_assignment
to support new roles and scopes
#11690
Conversation
eaf1c53
to
e8f5a26
Compare
this PR contains breaking changes, is it possible to be merged in the next major version 3.0 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?)
@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 |
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: |
if we default to workspace level and mark that as optional it's not a breaking change? |
@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. |
another way to make this PR: we could mark |
I like this id! and then we get proper validation on the id of the scope and no breaking change, this works for me! |
@katbyte I have refined this PR, now there is no breaking change. All acctest could pass. |
@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. |
@njuCZ - looks like all the tests are now filing:
|
72f4799
to
aba894a
Compare
@katbyte the acctest in teamcity could pass now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last comment i think
azurerm/internal/services/synapse/synapse_role_assignment_resource.go
Outdated
Show resolved
Hide resolved
799fb39
to
d104491
Compare
d104491
to
9b10400
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @njuCZ - LGTM aside from one comment which i'll fix now 🍰
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. |
fix #10141
in old api version, the only supported role is
Workspace Admin
,Sql Admin
andApache 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) .