-
Notifications
You must be signed in to change notification settings - Fork 64
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
dbforpostgresql: add FlexibleServerActiveDirectoryAdministrator #530
Conversation
/test-examples="examples/dbforpostgresql/flexibleserver-all-in-one.yaml" |
Signed-off-by: Yury Tsarev <yury@upbound.io>
ae925e0
to
c974056
Compare
/test-examples="examples/dbforpostgresql/flexibleserver-all-in-one.yaml" |
* dedicated uptest example * proper selector Signed-off-by: Yury Tsarev <yury@upbound.io>
17a3fac
to
3627623
Compare
/test-examples="examples/dbforpostgresql/flexibleserveractivedirectoryadministrator.yaml" |
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.
Thank you @ytsarev, LGTM.
@@ -645,7 +645,9 @@ var ExternalNameConfigs = map[string]config.ExternalName{ | |||
"azurerm_postgresql_flexible_server_firewall_rule": config.TemplatedStringAsIdentifier("name", "{{ .parameters.server_id }}/firewallRules/{{ .external_name }}"), | |||
"azurerm_postgresql_firewall_rule": config.TemplatedStringAsIdentifier("name", "/subscriptions/{{ .setup.configuration.subscription_id }}/resourceGroups/{{ .parameters.resource_group_name }}/providers/Microsoft.DBforPostgreSQL/servers/{{ .parameters.server_name }}/firewallRules/{{ .external_name }}"), | |||
"azurerm_postgresql_flexible_server": config.TemplatedStringAsIdentifier("name", "/subscriptions/{{ .setup.configuration.subscription_id }}/resourceGroups/{{ .parameters.resource_group_name }}/providers/Microsoft.DBforPostgreSQL/flexibleServers/{{ .external_name }}"), | |||
"azurerm_postgresql_virtual_network_rule": config.TemplatedStringAsIdentifier("name", "/subscriptions/{{ .setup.configuration.subscription_id }}/resourceGroups/{{ .parameters.resource_group_name }}/providers/Microsoft.DBforPostgreSQL/servers/{{ .parameters.server_name }}/virtualNetworkRules/{{ .external_name }}"), | |||
// /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroup/providers/Microsoft.DBforPostgreSQL/flexibleServers/myserver/administrators/objectId | |||
"azurerm_postgresql_flexible_server_active_directory_administrator": config.TemplatedStringAsIdentifier("", "/subscriptions/{{ .setup.configuration.subscription_id }}/resourceGroups/{{ .parameters.resource_group_name }}/providers/Microsoft.DBforPostgreSQL/flexibleServers/{{ .parameters.server_name }}/administrators/{{ .parameters.object_id }}"), |
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.
Just asking for understanding, why didn't we use the configuration in externalnamenottested.go
?
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.
@turkenf good question, configuration from the externalnamenottested.go
is the first thing i tried,
config.TemplatedStringAsIdentifier("object_id", "/subscriptions/{{ .setup.configuration.subscription_id }}/resourceGroups/{{ .parameters.resource_group_name }}/providers/Microsoft.DBforPostgreSQL/flexibleServers/{{ .parameters.server_name }}/administrators/{{ .external_name }}"),
in this case it was omitting objectId from the spec and using it as a name/externalname. ObjectId has uuid format like 13747b18-0617-4569-b94d-0cabe84934f8
and it would be quite obscure, non-obvious UX to make an AD object reference through the MR object name. That's why I decided to move it more straightforward spec.forProvider.objectId
. Hope it helps! :)
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.
Many thanks for the explanation @ytsarev 👍
Description of your changes
Add support for FlexibleServerActiveDirectoryAdministrator
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Please follow uptest run status below before the merge