Skip to content

Conversation

@simonkurtz-MSFT
Copy link
Member

The changes in this PR are largely refinements as this is still early on in the setup.

@simonkurtz-MSFT simonkurtz-MSFT requested a review from Copilot May 27, 2025 18:41
@github-actions
Copy link

Python 3.12 Test Results

84 tests  +3   84 ✅ +3   1s ⏱️ -1s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit c873843. ± Comparison against base commit 40b761c.

@github-actions
Copy link

Python 3.13 Test Results

84 tests  +3   84 ✅ +3   1s ⏱️ -1s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit c873843. ± Comparison against base commit 40b761c.

@simonkurtz-MSFT simonkurtz-MSFT added documentation Improvements or additions to documentation enhancement New feature or request labels May 27, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refines documentation and sample implementation details while updating policy handling and Bicep module parameter usage.

  • Adds tests for a new XML policy file reader and updates policy file handling to use read_policy_xml.
  • Refines Bicep templates by removing explicit location parameters and adding defaults where applicable.
  • Enhances README documentation by updating the sample list and improving descriptions.

Reviewed Changes

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

Show a summary per file
File Description
tests/python/test_utils.py Added tests for new read_policy_xml functionality
shared/python/utils.py Introduced read_policy_xml function
shared/python/apimtypes.py Fixed a minor spelling mistake in a comment
shared/bicep/modules/dns/v1/aca-dns-private-zone.bicep Updated location parameter to have a default value based on resourceGroup()
shared/bicep/modules/apim/v1/api.bicep Removed explicit location parameter
shared/bicep/modules/afd/v1/afd.bicep Removed explicit location parameter
samples/_TEMPLATE/main.bicep Updated resource naming and added placeholders for future modules
samples/_TEMPLATE/create.ipynb Refined notebook content and inline comments for clarity
infrastructure/simple-apim/create.ipynb Changed to use read_policy_xml for policy file reading
infrastructure/apim-aca/main.bicep Removed location parameters from container app modules
infrastructure/apim-aca/create.ipynb Updated policy file reading to use read_policy_xml
infrastructure/afd-apim/main.bicep Removed redundant location parameters in module definitions
infrastructure/afd-apim/create.ipynb Updated policy file reading to use read_policy_xml
README.md Enhanced documentation with an updated list of samples
Comments suppressed due to low confidence (4)

infrastructure/apim-aca/main.bicep:60

  • The explicit 'location' parameter has been removed from the container app modules; please document or verify that the module handles location appropriately in its internal logic.
    -    location: location

infrastructure/afd-apim/main.bicep:261

  • Confirm that the removal of the explicit 'location' parameter from acaDnsPrivateZoneModule aligns with the module's expectations and does not affect its functionality.
    -    location: location

infrastructure/afd-apim/main.bicep:271

  • Verify that the afdModule correctly infers a global scope without the explicit location parameter, ensuring that the intended configuration remains intact.
    -    location: 'global'

shared/bicep/modules/dns/v1/aca-dns-private-zone.bicep:11

  • Adding a default value for the location parameter is useful, but please ensure that using resourceGroup().location meets all deployment scenarios, especially for multi-region setups.
param location string = resourceGroup().location

@simonkurtz-MSFT simonkurtz-MSFT merged commit 92c32c2 into main May 27, 2025
7 checks passed
@simonkurtz-MSFT simonkurtz-MSFT deleted the refinements branch May 27, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants