[Question/Feedback]: Bicep - Pattern Module Tests - Relevancy/Questions - Sub Vending

[jtracey93]

Check for previous/existing GitHub issues

• I have checked for previous/existing GitHub issues

Description

So as part of developing/migrating the Bicep LZ/Sub Vending module to AVM, we have come across a few few interesting points that we need answers to to progress:

1. Min, Max, WAF aligned dont make sense for Sub Vending - how do we give an exception or handle this?
2. Required resources for testing, e.g. VWAN etc.
   • We have these pre-deployed currently in a tenant to speed up testing times, e.g. only have to deploy the spoke side, not the hubs
   • Can we carry on with this for this module?
   • Also hardcoding resource IDs in tests today, these would appear in examples (not ideal), have we got a known way to handle token replacement that we can expand for modules like sub vending
3. This module needs access to a billing account, which we do have in CSU world, so it can create new subscriptions and also requires tenant level access
4. We need to run our own pester tests on our module, where do these go?

Tasks

Preview Give feedback

No tasks being tracked yet.

Tasks

Preview Give feedback

1. Update Pester: Skip defaults|waf-aligned test folder check for ptn modules bicep-registry-modules#1470
   Type: AVM 🅰️ ✌️ Ⓜ️ +1
   
2. feat: Added post-deployment test support bicep-registry-modules#1036
   Type: AVM 🅰️ ✌️ Ⓜ️ +1
   
3. Update AVM specs: Expected Test Directories for Res modules only #851
   Type: AVM 🅰️ ✌️ Ⓜ️ Type: Documentation 📄 +2
   
4. [Question/Feedback]: Bicep - better document how to deal with module scripts #601
   Language: Bicep 💪 Type: Documentation 📄 Type: Question/Feedback 🙋 +3
   

[AlexanderSehr]

My 5 cents:

1. Min, Max, WAF aligned dont make sense for Sub Vending - how do we give an exception or handle this?

   Requires a test change. The deployment tests should be able to cope with it. Just make sure to have a main.test.bicep in whatever you want to call the test folder

2. Required resources for testing, e.g. VWAN etc.
   • We have these pre-deployed currently in a tenant to speed up testing times, e.g. only have to deploy the spoke side, not the hubs
   • Can we carry on with this for this module?
   • Also hardcoding resource IDs in tests today, these would appear in examples (not ideal), have we got a known way to handle token replacement that we can expand for modules like sub vending

   I'd suggest to deploy it as required. But there is a toggle on our workflows that allow you to disable the resource removal. But overall it doesn't matter that much. You're free to reference any existing resource that you see fit in your test. I would however recommend to reference them not via the full resourceID, but by referencing the .id property of a defined existing resource in the test file (e.g. via vWanResourceId: vWan.Id). In the readme it will then just how up like vWanResourceId: <vWanResourceId>.

3. This module needs access to a billing account, which we do have in CSU world, so it can create new subscriptions and also requires tenant level access

   Sure, let's do it. If the tenant allows us to deploy subscriptions, we could even bring back our good old alias module at some point. The only thing I'd raise is that we should expand on the removal logic that we have today which should, for subscriptions, 'remove' them and move the soft-deleted subscription in a 'decomissioned'-mg until gone (as per ALZ).

4. We need to run our own pester tests on our module, where do these go?

   Depends on which ones we're talking about. If we're talking static tests (that don't need a deployment) we support this today by just adding a Pester test script into the <pattern/module-folder>/tests/unit/ folder. For example avm\res\key-vault\vault\tests\unit\custom.tests.ps1. However, if you want to run post-deployment tests, it's not yet supported, though I'm happy to add support. We have an issue for that open for a while and just did not yet prioritize it. The idea here would be to enable the CI to search for Pester test scripts recursively in the corresponding e2e test folder (for example: avm\res\key-vault\vault\tests\e2e\max\custom.tests.ps1 or avm\res\key-vault\vault\tests\e2e\max\unit\custom.tests.ps1). Happy to push the request up, if desired.

[jtracey93]

Thanks @AlexanderSehr for the answers.

For No. 4 this is for post-deployment tests, so if we could or we can temporarily add this into the modules workflow ourselves, let us know. We can then use it as a test case when you intro the feature to the CI more formally.

For No. 3 do we need to give the existing identity these permissions or bring/introduce a new identity?

Let us know

[AlexanderSehr]

Thanks @AlexanderSehr for the answers.

For No. 4 this is for post-deployment tests, so if we could or we can temporarily add this into the modules workflow ourselves, let us know. We can then use it as a test case when you intro the feature to the CI more formally.

For No. 3 do we need to give the existing identity these permissions or bring/introduce a new identity?

Let us know

Hey @jtracey93,

for No.3 - the easiest would be option one, that is, giving the identity extra permissions. However, it's also the option that could be considered more risky. Introducing a separate identity comes with the added challenge that we need to enable the CI to use different sets of credentials may require an update of the CI as we inherit the crendentials into the shared workflow template. We 'may' be able to divert the flow to a different credential, effectively overwriting the default, but that is to be tested. If not we have to be smarter.

for No.4 - given the shared nature of our workflow template, it will hardly be possible to change something without impacting all workflows. And if we do that we may just as well just go ahead and introduce the feature into the shared template in the first place. The implementation should be pretty easy and not take longer than a few hours at most. We have all the bits and pieces we need already there and just have to re-use them in the correct way & test.

[AlexanderSehr]

Hey @jtracey93, regarding the post-deployment tests, i created this proposal: Azure/bicep-registry-modules#1036

[sebassem]

@AlexanderSehr @jtracey93 Thanks Alexander for the post deployment tests. I'm adding some additional points to the discussion as we keep developing the module.

• I suppose the current cleanup process does not handle subscriptions. I always getting it hanging on this step. If that's correct, is there any ongoing work to do that?
• Like the the post deployment tests implementation, I think it would be useful to have custom cleanup steps as well. For example, in our module, we need to cancel the subscription and move it to a specific management group and do some other tasks, like remove peerings from an existing vnet in our tenant.
• When executing the static tests, we noticed that it detected the roleAssignments parameter and gave an error that we need to implement the roleAssignments interface which would be a breaking change for us at the moment in sub-vending. Should this be checked for pattern modules or just resource modules? $resourceTypeIdentifier = ($moduleFolderPath -split '[\/|\\]{1}avm[\/|\\]{1}(res|ptn)[\/|\\]{1}')[2] -replace '\\', '/' # avm/res/<provider>/<resourceType>
• We need to discuss also how we are going to implement the identity which the publish CI will use to access the tenant we are using to create subscriptions

[eriqua]

1. Min, Max, WAF aligned dont make sense for Sub Vending - how do we give an exception or handle this?

@sebassem @jtracey93 just a heads-up for point 1 above has been addressed today by BRM PR 1622. Please let us know if you face additional unintended blockers with static validation.

[mbilalamjad]

Tagging @jtracey93, @sebassem to just bring this back to top of your inbox.

[sebassem]

thanks Bilal, we are still working with Alexander on other items in this list so I would say lets keep this issue open till we are done with the module to capture any new question/issue/feedback. I recently opened this issue to track one of the items that is blocking us as well

[AlexanderSehr]

Please note @jtracey93 & @sebassem, I created pull requests to further push the removal. One you already merged, which was about the correct resolution of resource Ids from the deployment, the next is this one Azure/bicep-registry-modules#1887, which enables us to control that the the alias resource is removed last, and finally we'd need to work on adding the explicit logic that handles the alias resource (e.g., cancel the subscription, create a management group to move it to (if not existing), and eventually move the subscription there). Whatever it is, @sebassem is looking into that one :) Once all pieces are in place we should be able to run an end-to-end test

[jtracey93]

closing this as I dont think this is needed anymore as we have published the sub vending module in Bicep via AVM