Add parameter to control deployment of OS image resources

[SvenAelterman]

No description provided.

[Copilot]

Pull request overview

Adds a new deployment toggle to allow skipping OS image (imaging) resource deployment from the research hub template, while still producing valid outputs when imaging is disabled.

Changes:

• Introduces a new deployImaging boolean parameter (defaulting to true).
• Makes the imaging module deployment conditional on deployImaging.
• Adjusts outputs (and one module parameter) to tolerate conditionally deployed modules.

Suppressed comments (1)

research-hub/main.bicep:577

• These outputs use a postfix ! (e.g., vmId!, imageDefinitionId!), which is inconsistent with the rest of the repository’s Bicep and may not be valid syntax. Since the modules are conditionally deployed, you can instead use safe-dereference + null-coalescing to provide the fallback values without relying on a non-null assertion.

output managementVmId string = logonType == 'ad' ? managementVmModule.?outputs.vmId! : 'N/A'
output managementVmUamiPrincipalId string = logonType == 'ad' ? managementVmModule.?outputs.uamiPrincipalId! : 'N/A'
output managementVmUamiClientId string = logonType == 'ad' ? managementVmModule.?outputs.uamiClientId! : 'N/A'

output imageDefinitionId string = deployImaging ? imagingModule.?outputs.imageDefinitionId! : 'N/A'

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

research-hub/main.bicep:558

• diskEncryptionSetModule is conditionally deployed (if (deployingVMs && useCMK)), but this line unconditionally forces an output access via .?outputs.id!. If the module isn't deployed (e.g., management VM is created while deployingVMs is false), this will produce null/invalid output access and can break the deployment. Guard the access with the same condition and fall back to an empty string (the management-vm module already defaults diskEncryptionSetId to '').

    diskEncryptionSetId: diskEncryptionSetModule.?outputs.id!

research-hub/main.bicep:576

• The .?outputs.<name>! pattern is both non-idiomatic for modules and easy to get wrong (safe-dereference applies only to the immediate property). Since these outputs are already guarded by logonType == 'ad' / deployImaging, prefer a straightforward access with a null-forgiving operator on the output value to satisfy the type checker.

output managementVmId string = logonType == 'ad' ? managementVmModule.?outputs.vmId! : 'N/A'
output managementVmUamiPrincipalId string = logonType == 'ad' ? managementVmModule.?outputs.uamiPrincipalId! : 'N/A'
output managementVmUamiClientId string = logonType == 'ad' ? managementVmModule.?outputs.uamiClientId! : 'N/A'

output imageDefinitionId string = deployImaging ? imagingModule.?outputs.imageDefinitionId! : 'N/A'

research-hub/main.bicep:133

• This newly added line appears to include a stray carriage return (CR) at the end of the line (mixed line endings). That can cause noisy diffs and may trip formatting/linting checks; please normalize it to the file’s existing line ending style (LF).

@description('If true, deploy OS image resources (Azure Image Builder / image definition) used by the environment.')

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

research-hub/main.bicep:575

• These outputs use managementVmModule.?outputs.<...>! syntax which is inconsistent with other module output references in this repo and is likely invalid Bicep (postfix ! is not used elsewhere). Since the ternary already guards on logonType == 'ad', reference managementVmModule.outputs directly.

output managementVmId string = logonType == 'ad' ? managementVmModule.?outputs.vmId! : 'N/A'
output managementVmUamiPrincipalId string = logonType == 'ad' ? managementVmModule.?outputs.uamiPrincipalId! : 'N/A'
output managementVmUamiClientId string = logonType == 'ad' ? managementVmModule.?outputs.uamiClientId! : 'N/A'

research-hub/main.bicep:577

• imagingModule.?outputs.imageDefinitionId! uses optional access plus a postfix ! that doesn't appear to be valid Bicep syntax. The output is already correctly guarded by deployImaging, so reference imagingModule.outputs.imageDefinitionId directly.

output imageDefinitionId string = deployImaging ? imagingModule.?outputs.imageDefinitionId! : 'N/A'

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

research-hub/main.bicep:468

• avdJumpBoxSessionHostModule is only deployed when !researchVmsAreSessionHosts, which is the same condition that deploys avdJumpBoxModule. The .?outputs ... ! pattern is therefore unnecessary here and makes the dependency harder to reason about; use the normal .outputs references.

    hostPoolName: avdJumpBoxModule.?outputs.hostPoolName!
    hostPoolToken: avdJumpBoxModule.?outputs.hostPoolRegistrationToken!

research-hub/main.bicep:462

• diskEncryptionSetModule is conditional, but diskEncryptionSetId is passed via diskEncryptionSetModule.?outputs.id ?? ''. Prefer explicitly aligning the reference with the module's deployment condition (and using standard .outputs) so the template behavior is clearer and avoids relying on safe-navigation syntax.

    diskEncryptionSetId: diskEncryptionSetModule.?outputs.id ?? ''

research-hub/main.bicep:585

• The output ternaries are already guarding access to conditional modules, so the .?outputs ... ! pattern adds risk (unsupported syntax) without providing additional safety. Use standard .outputs references inside the guarded branches.

output managementVmId string = logonType == 'ad' ? managementVmModule.?outputs.vmId! : 'N/A'
output managementVmUamiPrincipalId string = logonType == 'ad' ? managementVmModule.?outputs.uamiPrincipalId! : 'N/A'
output managementVmUamiClientId string = logonType == 'ad' ? managementVmModule.?outputs.uamiClientId! : 'N/A'

output imageDefinitionId string = deployImaging ? imagingModule.?outputs.imageDefinitionId! : 'N/A'

research-hub/main.bicep:365

• Using encryptionKeysModule.?outputs.keys! introduces safe-navigation + null-assertion on a module output. Elsewhere in the repo, conditional modules are referenced via standard .outputs guarded by the same condition; using the standard pattern here reduces the risk of relying on unsupported/unstable syntax.

This issue also appears in the following locations of the same file:

• line 460
• line 467
• line 581

var kvEncryptionKeys = useCMK ? reduce(encryptionKeysModule.?outputs.keys!, {}, (cur, next) => union(cur, next)) : {}

research-hub/main.bicep:567

• The management VM can be deployed even when diskEncryptionSetModule is not (e.g., logonType == 'ad' but deployingVMs is false with current logic). Passing DES via diskEncryptionSetModule.?outputs.id ?? '' hides this mismatch; align the expression to the DES module's condition so it's obvious when CMK encryption is actually applied.

    logonType: logonType
    diskEncryptionSetId: diskEncryptionSetModule.?outputs.id ?? ''
  }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (5)

research-hub/main.bicep:461

• diskEncryptionSetModule.?outputs.id ?? '' relies on the same .?outputs syntax that isn’t used elsewhere in the repo. Since this session host module only runs when jumpBoxSessionHostCount > 0, you can use a ternary on useCMK and reference diskEncryptionSetModule.outputs.id directly (matching patterns used in other templates).

    diskEncryptionSetId: diskEncryptionSetModule.?outputs.id ?? ''

research-hub/main.bicep:468

• avdJumpBoxModule.?outputs.*! is an uncommon pattern in this repo and should be unnecessary here because avdJumpBoxModule is always deployed whenever the session host module is deployed (!researchVmsAreSessionHosts). Using direct avdJumpBoxModule.outputs.* keeps things consistent and avoids relying on optional chaining / non-null assertion syntax.

    hostPoolName: avdJumpBoxModule.?outputs.hostPoolName!
    hostPoolToken: avdJumpBoxModule.?outputs.hostPoolRegistrationToken!

research-hub/main.bicep:566

• diskEncryptionSetModule.?outputs.id ?? '' again introduces the .?outputs pattern that’s otherwise absent from the repo. Since managementVmModule is only deployed when logonType == 'ad' (and deployingVMs is true in that case), you can gate on useCMK and use diskEncryptionSetModule.outputs.id directly.

    diskEncryptionSetId: diskEncryptionSetModule.?outputs.id ?? ''

research-hub/main.bicep:585

• The outputs use module.?outputs.*!, which is an uncommon/unique pattern in this repo and may reduce compatibility with the Bicep tooling version used in CI. A consistent approach elsewhere is to use a conditional operator around module.outputs.* without optional chaining (e.g. logonType == 'ad' ? managementVmModule.outputs.vmId : 'N/A').

output managementVmId string = logonType == 'ad' ? managementVmModule.?outputs.vmId! : 'N/A'
output managementVmUamiPrincipalId string = logonType == 'ad' ? managementVmModule.?outputs.uamiPrincipalId! : 'N/A'
output managementVmUamiClientId string = logonType == 'ad' ? managementVmModule.?outputs.uamiClientId! : 'N/A'

output imageDefinitionId string = deployImaging ? imagingModule.?outputs.imageDefinitionId! : 'N/A'

research-hub/main.bicep:365

• encryptionKeysModule.?outputs.keys! introduces a unique optional-chaining/non-null-assert pattern that isn’t used elsewhere in the repo and may reduce Bicep CLI compatibility. Other templates (e.g. research-spoke/main.bicep:439) safely reference conditional module outputs inside a ternary without .?outputs/!.

Consider aligning to that pattern here to keep the template consistent and avoid depending on newer syntax.

This issue also appears in the following locations of the same file:

• line 461
• line 467
• line 566
• line 581

var kvEncryptionKeys = useCMK ? reduce(encryptionKeysModule.?outputs.keys!, {}, (cur, next) => union(cur, next)) : {}