Azure / bicep-registry-modules

Bicep registry modules
MIT License
508 stars 354 forks source link

[Modules] Extend `Microsoft.Sql\servers` with `administrators` child resource #2430

Open eriqua opened 2 years ago

eriqua commented 2 years ago

Create a new child module for the Microsoft.Sql/servers/administrators resource type as per the service documentation and our contribution guide.

AlexanderSehr commented 5 months ago

Hey @bryansan-msft , I just migrated this issue over from CARML. Please take a look and triage if still relevant :)

peterbud commented 1 month ago

There is already an administrators property for the server.

I'm I right, that this can be set up only at server creation, and your issue is that in order to be able to modify administrators after creation, we'd need a separate module for that?

AlexanderSehr commented 4 weeks ago

Hey @peterbud, it may be one of them unfortunate cases where a resource provider has multiple ways to deploy a single feature (like with the subnets for a vnet, or routes for a route table). In these cases there is both a property (as you called out) and a child-module available. Now, for the aforementioned cases I'd recommend to not use the property but instead a dedicated child module because

  1. The child module will eventually be published as its own instance
  2. The dedicated modules often (not always) come with additional features

However, as per your statement, I presume this will not necessarily be an option for the SQL-Sever module, correct? I'm basing this statement on the sentence 'can be set up only at server creation'. We can of course also have both the property & the child-module - but - this would only make sense if the child-module does indeed have additional features implemented, that the property wouldn't have. If not, one could still argue it would be nice to publish the extra module eventually - but I'm not sure if it would be worth the extra deployment.

Looking forward to your thoughts on the matter :)

peterbud commented 1 week ago

Hi @AlexanderSehr ,

I have checked, and the server's administrators property is different than the server/administrator child module, so I believe we need to have both available.

peterbud commented 1 week ago

I have spent more time to understand this specific administrators child resource together with the azureADOnlyAuthentications child resource (see here)

It seems that at the creation of a server you MUST use one of the two inline properties:

If you don't use one of the two above you cannot create a server, even when you supply the same data at the child administrators resource plus the child 'azureADOnlyAuthentications'. You'll only get an error Invalid value given for parameter Login. Specify a valid parameter value

More you can read also at the Bicep issue here

So it seems to me that the administrators and azureADOnlyAuthentications child resources are ONLY suitable to change an existing server deployment, like changing the administrators or flipping the AAD only authentication.

So if the above is true: does it make sense to create these child resources, knowing that they could be only used in a standalone way?

AlexanderSehr commented 1 week ago

Hey @peterbud, thanks for the comprehensive explanation. It is a good question. In this case do lean towards having both - but not because it may do us any good now, but rather that these child modules would indeed be published as standalone modules once we are able to enable the feature in the future. However, me leaning towards something in the future does not mean you have to agree 😄 We could just as well keep this issue open and tag it with the long term label to implement it once child-modules are actually published. So, I guess it's about closing it either in the short- or long term. It's really unfortunate to hear that the RP is implemented in this way, i.e., that the child-resources can only be used to update an existing instance as opposed to being a replacement for the property. In any case, if the two overlap heavilty, we may be able to implement the change in a way that the same property does not have to be specified twice, but that the user can specify an administrator object/array as an input parameter which would be passed into the resource deployment, and later, when there may be an administrator child module, pass the same object (optionally with more properties if available) to that one.

peterbud commented 1 week ago

Ok, let's tag it with a long term label.