feat: Added support for Discrimintor-based variants to readme utility #4146
Conversation
microsoft-github-policy-service
bot
added
the
Needs: Triage 🔍
AlexanderSehr
added
Needs: Core Team 🧞
microsoft-github-policy-service
bot
added
the
Type: AVM 🅰️ ✌️ Ⓜ️
|
@AlexanderSehr This looks good to me is there anything we should do to prepare for this change? |
| @@ -1608,6 +1608,249 @@ The outbound rule. The name of the rule is the object key. | |||
|
|
|||
| - Required: Yes | |||
| - Type: object | |||
| - Type-deciding property: type | |||
There was a problem hiding this comment.
Waiting for readmes to be regenerated after agreeing on https://github.com/Azure/bicep-registry-modules/pull/4146/files?file-filters%5B%5D=.ps1&file-filters%5B%5D=.psm1&show-viewed-files=true#r1995727675
| | :-- | :-- | :-- | | ||
| | [`category`](#parameter-workspaceconfigurationnetworkoutboundrules>any_other_property<type-fqdncategory) | string | Category of a managed network Outbound Rule of the workspace hub. | | ||
|
|
||
| ### Parameter: `workspaceConfiguration.networkOutboundRules.>Any_other_property<.type-FQDN.destination` |
There was a problem hiding this comment.
Is it intended to have variant parameters with the same header level 3 (###)? Shouldn't they be level 4 (####)?
Not blocking if complex to achieve, as I understand they may be multiple levels of nesting (e.g. variants in parameters containing variants as well). Just wanted to bring this up
There was a problem hiding this comment.
It is a great question and it took me a while to understand why it's implemented in the way it is. If you collapse all sections in a readme (like this one) you'll see that the Parameters section is on nesting level 2 (##) and all parameters below it are on level 3 (###). This may seem odd a first, but the hierachy is actually not expressed via the # but the title which gets continuously appended the further 'in' you go.
I can't recall why this was done for the parameters originally - but - my best guess is that we'd run the hazard of having parameters being nested like ############### storageAccount.blobService.container.blob.immatbilitypolicy.somethingelse.and.so.on.lastItem. The alternative would be just to have the headers and end on ############### lastItem - but that would not be unique (so it cannot be referenced).
The other thing is that something like ################### title can look super odd in Markdown. I can't recall in which Wiki I've seen it, but I've seen these titles being rendered smaller and smaller the deeper you go. Naturally with the text below it, that's not a header, being at normal size 😄.
Anyways, these are just some guesses.
| | [`useExisting`](#variant-networkingconfigurationnetworktype-useexisting) | | | ||
|
|
||
| ### Variant: `networkingConfiguration.networkType-createNew` | ||
|
|
There was a problem hiding this comment.
We probably miss a @description on the discriminator every time we have 2 empty lines after the variant header. Should we have this checked? Not for this PR, just again wanted to bring it up (maybe it's already producing a warning)
There was a problem hiding this comment.
They do miss it, yes. So the function works as expected.
That being said we certainly do have a spec that say it's a MUST (ref) to implement certain metadata.
I don't think we have a test for it yet though - so I agree it should not be part of this PR, but be implemented. The question, as with the nullable etc. will be whether the test should be enforced or not 😄 But maybe that's something we can decide after the test was implemented and run across the modules to see how many would need fixing. But I guess it's a lot.
Co-authored-by: Erika Gressi <[email protected]>
Co-authored-by: Erika Gressi <[email protected]>
Co-authored-by: Erika Gressi <[email protected]>
Description
Set-ModuleReadMescript would throw an exception during the generation, returning the error that would hit the Pester-Output which in turn would pollute the test results the GitHub-Output function then failed to interpret and publish. The fix essentially catches this exception and fails the test with a proper error message, pointing to the actual cause of the error (e.g., a parameter not having a description category specified). See screenshots at the buttom for a before-after comparison.Required module fixes
Set-ModuleReadMescript lacked 2 lines of code that would reset the two values to$nullin incase no min or max value would be available for the parameter. Without this reset, the logic re-used the values of the pervious parameter. The effect: As soon as one parameter defined a min & max value - all subsequent parameters would show the same too (which is obviously very very wrong).Closes #4128
Pipeline Reference
Type of Change
version.json:version.json.version.json.Appendix
Error before the fix
Error after the fix