feat: add userDefinedTypes for secret and key - avm/res/key-vault/vault
#1123
Conversation
microsoft-github-policy-service
bot
added
Needs: Triage 🔍
fblix
requested review from
AlexanderSehr and
eriqua
and removed request for
AlexanderSehr and
eriqua
| @description('Optional. Key rotation policy properties object.') | ||
| param rotationPolicy object? | ||
| @description('Optional. Sets the attributes of the secret.') | ||
| param keyProperties keyType |
There was a problem hiding this comment.
This is a pretty major change to the convention of how modules are written - and I would expect that many of our tests would not be compatible if changed without previous CI updates.
Overall I'm not sure if we'd want a UDT of an entire resource in that resource's module. As you might have noticed yourself, referencing the properties etc. get's quite messy, defaults cannot be set in the type and hence are also not picked up by the calling module as defaults anymore.
Mind you, the parameters themselves are already a User-defined-type with linter and everything. By moving all properties into a UDT, you just shift the default Linter support to a UDT linter support.
We've seen a pattern similar to this a couple years back for modules used at a customer where all modules only implemented a properties parameter so that maintenance would be at a minimum. This was strongly discouraged by the PG, and while not the same here, it's moving us closer in that direction.
I'd suggest to brainstorm this a bit, but from a first impression I'd suggest to only have the UDT in the calling module (and in the future once out of preview store it in a types file) for that type (e.g. keyType in the key vault's /vault/main.bicep but not the /vault/key/main.bicep).
Don't change anything now, but have a chat - ideally including also @ChrisSidebotham & @eriqua as this should be a references for other contributors too.
There was a problem hiding this comment.
I see what you mean, main goal with my implementation was to do it similarly as we did with vault/accessPolicies already, too. I think we should define a line up till which it makes sense to move stuff into a UDT, what do you think?
There was a problem hiding this comment.
I agree; I wouldn't change the child module to be a single object input param. Having a type in the parent file for the child resource is fine as this enables the benefits of having a UDT. But the parameters should still be passed independently.
This is what I have done in the Service Bus Module
There was a problem hiding this comment.
I like the approach, @ChrisSidebotham !
should I do the same for the keyvault module then, @AlexanderSehr ?
There was a problem hiding this comment.
I see what you mean, main goal with my implementation was to do it similarly as we did with
vault/accessPoliciesalready, too. I think we should define a line up till which it makes sense to move stuff into a UDT, what do you think?
It's actually not as intense for access policies @fblix. One of the properties (not the properties themselves) is an array of objects (the policies). Hence the UDT describes how this input array looks like:
There was a problem hiding this comment.
I like the approach, @ChrisSidebotham ! should I do the same for the keyvault module then, @AlexanderSehr ?
Its my prefered approach as that way (when) we get functionality to deploy child resources, they are still fully functioning modules and the type is to assist someone who is deploying the parent resources with interfaces or child modules.
There was a problem hiding this comment.
Agreed. The UDT would 'only' be in the parent.
Mind you, in the future, when we can import resource-udts as well as custom-udts from an external file, we can import nested properties via this central file instead of repeating it potentially over and over. Not relevant yet, but will be eventually.
There was a problem hiding this comment.
Nothing to add from my side. 100% agree with the suggested approach. Leverage UDT for child modules in the parent, but keep the first line of individual input parameters at child module level.
There was a problem hiding this comment.
@fblix - Are you okay to see this through?
There was a problem hiding this comment.
I have ported these changes to #1986
ChrisSidebotham
added
Needs: Author Feedback 👂
matebarabas
changed the title
avm/res/key-vault/vault
…ult` (#1986) ## Description I have migrated changes from 1123 into my own feature branch to close and completes the related tasks to 1123 #closes 1122 - adds tests for ec and rsa kty key parameters - adds checks for dependencies between keySize and curveName key parameters - Expanded UDT for Secrets - Exapanded UDT for Keys - added support for releasePolicy for key - resolves the conflicts from #1123 <!-- >Thank you for your contribution ! > Please include a summary of the change and which issue is fixed. > Please also include the context. > List any dependencies that are required for this change. Fixes #123 Fixes #456 Closes #123 Closes #456 --> ## Pipeline Reference <!-- Insert your Pipeline Status Badge below --> | Pipeline | | -------- | | [](https://github.com/ChrisSidebotham/bicep-registry-modules/actions/workflows/avm.res.key-vault.vault.yml) | ## Type of Change <!-- Use the check-boxes [x] on the options that are relevant. --> - [x] Update to CI Environment or utlities (Non-module effecting changes) - [ ] Azure Verified Module updates: - [ ] Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in `version.json`: - [ ] Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description. - [ ] The bug was found by the module author, and no one has opened an issue to report it yet. - [ ] Feature update backwards compatible feature updates, and I have bumped the MINOR version in `version.json`. - [x] Breaking changes and I have bumped the MAJOR version in `version.json`. - [ ] Update to documentation ## Checklist - [ ] I'm sure there are no other open Pull Requests for the same update/change (#1123 IS OPEN AND SHOULD BE DISCARDED) - [x] I have run `Set-AVMModule` locally to generate the supporting module files. - [x] My corresponding pipelines / checks run clean and green without any errors or warnings <!-- Please keep up to day with the contribution guide at https://aka.ms/avm/contribute/bicep -->
|
Closing as completed via #1986 |
microsoft-github-policy-service
bot
added
Needs: Attention 👋
|
Note The "Needs: Author Feedback 👂" label was removed and the "Needs: Attention 👋" label was added as per ITA11. |
](https://github.com/ChrisSidebotham/bicep-registry-modules/actions/workflows/avm.res.key-vault.vault.yml){kind=link}
…lt/vault` (Azure#1986) ## Description I have migrated changes from 1123 into my own feature branch to close and completes the related tasks to 1123 #closes 1122 - adds tests for ec and rsa kty key parameters - adds checks for dependencies between keySize and curveName key parameters - Expanded UDT for Secrets - Exapanded UDT for Keys - added support for releasePolicy for key - resolves the conflicts from Azure#1123 <!-- >Thank you for your contribution ! > Please include a summary of the change and which issue is fixed. > Please also include the context. > List any dependencies that are required for this change. Fixes Azure#123 Fixes Azure#456 Closes Azure#123 Closes Azure#456 --> ## Pipeline Reference <!-- Insert your Pipeline Status Badge below --> | Pipeline | | -------- | | [](https://github.com/ChrisSidebotham/bicep-registry-modules/actions/workflows/avm.res.key-vault.vault.yml) | ## Type of Change <!-- Use the check-boxes [x] on the options that are relevant. --> - [x] Update to CI Environment or utlities (Non-module effecting changes) - [ ] Azure Verified Module updates: - [ ] Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in `version.json`: - [ ] Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description. - [ ] The bug was found by the module author, and no one has opened an issue to report it yet. - [ ] Feature update backwards compatible feature updates, and I have bumped the MINOR version in `version.json`. - [x] Breaking changes and I have bumped the MAJOR version in `version.json`. - [ ] Update to documentation ## Checklist - [ ] I'm sure there are no other open Pull Requests for the same update/change (Azure#1123 IS OPEN AND SHOULD BE DISCARDED) - [x] I have run `Set-AVMModule` locally to generate the supporting module files. - [x] My corresponding pipelines / checks run clean and green without any errors or warnings <!-- Please keep up to day with the contribution guide at https://aka.ms/avm/contribute/bicep -->
Description
Closes #1122
Pipeline Reference
Type of Change
version.json:version.json.version.json.Checklist
Set-AVMModulelocally to generate the supporting module files.