Merge Main into feature branch to pull latest changes#150
Closed
ncheruvu-MSFT wants to merge 5 commits intofeature/costing-entra-appidfrom
Closed
Merge Main into feature branch to pull latest changes#150ncheruvu-MSFT wants to merge 5 commits intofeature/costing-entra-appidfrom
ncheruvu-MSFT wants to merge 5 commits intofeature/costing-entra-appidfrom
Conversation
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: simonkurtz-MSFT <84809797+simonkurtz-MSFT@users.noreply.github.com> Co-authored-by: Simon Kurtz <simonkurtz@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR syncs in broad repo updates, primarily migrating Python linting from Pylint to Ruff, adding an APIM costing/showback sample, and enhancing several samples/modules (including secure blob access via managed identity and new APIM diagnostics Bicep).
Changes:
- Replace Pylint with Ruff across local scripts, VS Code settings, CI workflow, and project configuration.
- Add the new
samples/costing/sample (Bicep, workbook, KQL, docs) and update repo docs/assets to reference it. - Update multiple notebooks/policies/Bicep modules (notably secure-blob-access and shared APIM modules), plus assorted refactors (imports, status-code constants, formatting).
Reviewed changes
Copilot reviewed 54 out of 64 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates dev tooling dependencies (swap pylint → ruff) and lock content. |
| pyproject.toml | Adds Ruff config and switches dev dependency from pylint to ruff. |
| .pylintrc | Removes pylint configuration (pylint is no longer used). |
| .github/workflows/python-tests.yml | CI lint step migrates from pylint to ruff; artifacts/summary updated. |
| .github/python.instructions.md | Updates repo guidance to reference Ruff config and expectations. |
| .github/copilot-instructions.md | Adds/expands repo-wide conventions and explicitly documents Ruff usage. |
| tests/python/run_ruff.sh | New/updated Ruff runner script with report generation. |
| tests/python/run_ruff.ps1 | Adds PowerShell Ruff runner with reports and summary output. |
| tests/python/run_pylint.ps1 | Removes Pylint PowerShell runner. |
| tests/python/check_python.sh | Switches “full checks” to call Ruff instead of Pylint. |
| tests/python/check_python.ps1 | Switches “full checks” to call Ruff instead of Pylint. |
| tests/README.md | Updates documentation to describe Ruff-based linting workflow. |
| start.sh | Developer CLI menu: “Run pylint” → “Run ruff”; updates command invoked. |
| start.ps1 | Developer CLI menu: “Run pylint” → “Run ruff”; updates command invoked. |
| .vscode/settings.json | Configures Ruff formatter/code actions; disables pylint extension integration. |
| .vscode/extensions.json | Recommends Ruff extension; marks pylint extension unwanted. |
| .gitignore | Ignores Ruff report output path (comment still mentions pylint). |
| shared/python/apimrequests.py | Uses HttpStatusCode constants for response handling. |
| shared/python/charts.py | Replaces 200 literals with HttpStatusCode.OK in chart logic. |
| shared/python/console.py | Refactors console logging helpers; formatting/constants cleanup. |
| shared/python/infrastructures.py | Uses HttpStatusCode.OK for connectivity check; import reordering. |
| shared/python/show_infrastructures.py | Style/quoting cleanup and small formatting refactor. |
| shared/python/utils.py | Minor refactor in placeholder replacement loop variable names. |
| shared/bicep/modules/apim/v1/api.bicep | Adds dependency ordering for APIM tag resources. |
| shared/bicep/modules/apim/v1/diagnostics.bicep | Adds new module to configure APIM diagnostics/logging (LAW + App Insights). |
| shared/azure-roles.json | Adds Storage File Data Privileged Contributor role ID constant. |
| samples/secure-blob-access/main.bicep | Disables shared key access; removes key named value; adds blob delegator role assignment. |
| samples/secure-blob-access/pf-create-sas-token.xml | Switches to User Delegation SAS generation using managed identity. |
| samples/secure-blob-access/blob-get-operation.xml | Removes shared key usage; ties operation to new SAS fragment output. |
| samples/secure-blob-access/upload-sample-files.bicep | Adds additional role + storageAccountSettings for deployment script backing storage. |
| samples/secure-blob-access/create.ipynb | Adds manual blob upload step; however current state has missing required notebook variables (breaks execution). |
| samples/secure-blob-access/README.md | Updates narrative to User Delegation SAS + shared key disabled. |
| samples/load-balancing/create.ipynb | Refactors formatting/status codes; however current state has missing required notebook variables (breaks execution). |
| samples/oauth-3rd-party/create.ipynb | Refactors to explicit imports and clearer formatting. |
| samples/general/create.ipynb | Refactors to explicit imports and clearer formatting. |
| samples/azure-maps/create.ipynb | Refactors to explicit imports and clearer formatting. |
| samples/authX/create.ipynb | Refactors to explicit imports and clearer formatting. |
| samples/authX-pro/create.ipynb | Refactors to explicit imports and clearer formatting. |
| samples/_TEMPLATE/create.ipynb | Refactors template to explicit imports/formatting; trims example test boilerplate. |
| samples/costing/main.bicep | New costing sample infra (LAW/AppInsights/Workbook/Diagnostics/exports wiring). |
| samples/costing/cost-export.bicep | New subscription-scope cost export module. |
| samples/costing/workbook.json | New Azure Monitor workbook template for cost/showback analytics. |
| samples/costing/README.md | New sample documentation for costing/showback. |
| samples/costing/budget-alert-threshold.kql | New KQL query template for budget/threshold alerting. |
| samples/costing/verify-log-ingestion.kql | New KQL to validate APIM gateway logs ingestion. |
| samples/costing/screenshots/README.md | Documents expected screenshots/artifacts for costing sample. |
| assets/diagrams/Infrastructure-Sample-Compatibility.svg | Updates compatibility matrix to include “Costing” sample row. |
| assets/APIM-Samples-Slide-Deck.html | Adds slide deck; contains one outdated reference to pylint. |
| README.md | Updates docs to reference Ruff and adds costing sample entry/credits. |
| tests/python/test_utils.py | Removes pylint suppression comments now that pylint is removed. |
| tests/python/test_infrastructures.py | Adjusts tests (type annotation comparisons, removes pylint suppression comments). |
| tests/python/test_show_infrastructures.py | Minor variable renames in comprehensions for clarity. |
| tests/python/test_charts.py | Uses HttpStatusCode.OK in assertions to match production code changes. |
| tests/python/conftest.py | Removes pylint-specific suppression comment and cleans import block. |
| <li><div class="bullet"></div><div><strong>Dev Container</strong> — One-click Codespaces or local Dev Container setup</div></li> | ||
| <li><div class="bullet"></div><div><strong>Developer CLI</strong> — Interactive menu for setup, testing & verification</div></li> | ||
| <li><div class="bullet"></div><div><strong>Cross-Platform</strong> — Windows, Linux, macOS support</div></li> | ||
| <li><div class="bullet"></div><div><strong>Tested</strong> — CI with pytest, pylint, and coverage</div></li> |
There was a problem hiding this comment.
This slide deck still states the CI uses "pylint", but the repo has migrated to Ruff (e.g., workflow and tooling scripts). Update this text to avoid misleading users.
Suggested change
| <li><div class="bullet"></div><div><strong>Tested</strong> — CI with pytest, pylint, and coverage</div></li> | |
| <li><div class="bullet"></div><div><strong>Tested</strong> — CI with pytest, Ruff, and coverage</div></li> |
Comment on lines
93
to
95
| "tests = ApimTesting(\"Load Balancing Sample Tests\", sample_folder, deployment)\n", | ||
| "\n", | ||
| "# Determine endpoints, URLs, etc. prior to test execution\n", |
There was a problem hiding this comment.
This test cell uses sample_folder and rg_name (and later apim_gateway_url), but the notebook no longer defines sample_folder/rg_name/nb_helper in the initialization section. Re-add the SYSTEM CONFIGURATION/API definition block so these variables exist before running tests.
Comment on lines
31
to
+35
| "index = 1\n", | ||
| "apim_sku = APIM_SKU.BASICV2 # Options: 'BASICV2', 'STANDARDV2', 'PREMIUMV2'\n", | ||
| "apim_sku = APIM_SKU.STANDARDV2 # Options: 'BASICV2', 'STANDARDV2', 'PREMIUMV2'\n", | ||
| "deployment = INFRASTRUCTURE.SIMPLE_APIM # Options: see supported_infras below\n", | ||
| "api_prefix = 'blob-'\n", | ||
| "tags = ['secure-blob-access', 'valet-key', 'storage', 'jwt', 'authz']\n", | ||
| "\n", | ||
| "\n", | ||
| "\n", | ||
| "# ------------------------------\n", | ||
| "# SYSTEM CONFIGURATION\n", | ||
| "# ------------------------------\n", | ||
| "\n", | ||
| "# Create the notebook helper with JWT support\n", | ||
| "sample_folder = 'secure-blob-access'\n", | ||
| "rg_name = get_infra_rg_name(deployment, index)\n", | ||
| "supported_infras = [INFRASTRUCTURE.AFD_APIM_PE, INFRASTRUCTURE.APIM_ACA, INFRASTRUCTURE.APPGW_APIM, INFRASTRUCTURE.APPGW_APIM_PE, INFRASTRUCTURE.SIMPLE_APIM]\n", | ||
| "nb_helper = utils.NotebookHelper(sample_folder, rg_name, rg_location, deployment, supported_infras, True, index = index, apim_sku = apim_sku)\n", | ||
| "\n", | ||
| "# Blob storage configuration\n", | ||
| "container_name = 'hr-assets'\n", | ||
| "file_name = 'hr.txt'\n", | ||
| "\n", | ||
| "# Define the APIs and their operations and policies\n", | ||
| "\n", | ||
| "# Set up the named values\n", | ||
| "nvs: List[NamedValue] = [\n", | ||
| " NamedValue(nb_helper.jwt_key_name, nb_helper.jwt_key_value_bytes_b64, True),\n", | ||
| " NamedValue('HRMemberRoleId', Role.HR_MEMBER)\n", | ||
| "]\n", | ||
| "\n", | ||
| "# Load policy fragment definitions\n", | ||
| "pf_authx_hr_member_xml = utils.read_policy_xml('pf-authx-hr-member.xml', {\n", | ||
| " 'jwt_signing_key' : nb_helper.jwt_key_name,\n", | ||
| " 'hr_member_role_id' : 'HRMemberRoleId'\n", | ||
| "}, sample_folder)\n", | ||
| "\n", | ||
| "pf_create_sas_token_xml = utils.read_policy_xml('pf-create-sas-token.xml', sample_name = sample_folder)\n", | ||
| "pf_check_blob_existence_via_mi = utils.read_policy_xml('pf-check-blob-existence-via-managed-identity.xml', sample_name = sample_folder)\n", | ||
| "\n", | ||
| "# Define policy fragments\n", | ||
| "pfs: List[PolicyFragment] = [\n", | ||
| " PolicyFragment('AuthX-HR-Member', pf_authx_hr_member_xml, 'Authenticates and authorizes users with HR Member role.'),\n", | ||
| " PolicyFragment('Create-Sas-Token', pf_create_sas_token_xml, 'Creates a SAS token to use with access to a blob.'),\n", | ||
| " PolicyFragment('Check-Blob-Existence-via-Managed-Identity', pf_check_blob_existence_via_mi, 'Checks whether the specified blob exists at the blobUrl. A boolean value for blobExists will be available afterwards.')\n", | ||
| "]\n", | ||
| "\n", | ||
| "# Load API policy\n", | ||
| "pol_blob_get = utils.read_and_modify_policy_xml('blob-get-operation.xml', {\n", | ||
| " 'container_name': container_name\n", | ||
| "}, sample_folder)\n", | ||
| "\n", | ||
| "# Define template parameters for blob name\n", | ||
| "blob_template_parameters = [\n", | ||
| " {\n", | ||
| " \"name\": \"blob-name\",\n", | ||
| " \"description\": \"The name of the blob to access\",\n", | ||
| " \"type\": \"string\",\n", | ||
| " \"required\": True\n", | ||
| " }\n", | ||
| "]\n", | ||
| "\n", | ||
| "# Define API operations\n", | ||
| "\n", | ||
| "# API 1: Secure Blob Access API\n", | ||
| "secure_blob_path = f'/{api_prefix}secure-files'\n", | ||
| "secure_blob_get = GET_APIOperation2('GET', 'GET', '/{blob-name}', 'Gets the blob access valet key info', pol_blob_get, templateParameters=blob_template_parameters)\n", | ||
| "secure_blob = API('secure-blob-access', 'Secure Blob Access API', f'/{api_prefix}secure-files', 'API for secure access to blob storage using the valet key pattern', operations = [secure_blob_get], tags = tags)\n", | ||
| "\n", | ||
| "# APIs Array\n", | ||
| "apis: List[API] = [secure_blob]\n", | ||
| "\n", | ||
| "print_ok('Notebook initialized')" | ||
| "tags = ['secure-blob-access', 'valet-key', 'storage', 'jwt', 'authz']" |
There was a problem hiding this comment.
The notebook initialization cell no longer defines required variables (e.g., nb_helper, rg_name, apis, nvs, pfs, container_name, file_name), but later cells reference them. As-is, running the notebook will raise NameError; reintroduce the SYSTEM CONFIGURATION section (as in samples/_TEMPLATE/create.ipynb) and define these values before the deployment cell.
Comment on lines
55
to
+60
| "# Build the bicep parameters\n", | ||
| "bicep_parameters = {\n", | ||
| " 'apis' : {'value': [api.to_dict() for api in apis]},\n", | ||
| " 'namedValues' : {'value': [nv.to_dict() for nv in nvs]},\n", | ||
| " 'policyFragments' : {'value': [pf.to_dict() for pf in pfs]},\n", | ||
| " 'containerName' : {'value': container_name},\n", | ||
| " 'blobName' : {'value': file_name}\n", | ||
| " 'containerName' : {'value': container_name}\n", |
There was a problem hiding this comment.
This deployment cell builds bicep parameters from variables (apis/nvs/pfs/container_name) and calls nb_helper, but those variables are not defined anywhere in the current notebook file. Define them in the init cell (or in a preceding cell) before constructing bicep_parameters.
Comment on lines
+109
to
+115
| "# Assign Storage Blob Data Contributor to the current user on the storage account\n", | ||
| "storage_account_resource_id = (\n", | ||
| " f'/subscriptions/{subscription_id}/resourceGroups/{rg_name}'\n", | ||
| " f'/providers/Microsoft.Storage/storageAccounts/{storage_account_name}'\n", | ||
| ")\n", | ||
| "blob_contributor_role = 'ba92f5b4-2d11-453d-a403-e96b0029c9fe'\n", | ||
| "\n", |
There was a problem hiding this comment.
This upload cell uses rg_name and file_name, but neither is defined in the notebook (and rg_name is required to build the scope resource ID). Define rg_name (e.g., via get_infra_rg_name(deployment, index)) and file_name before this cell, or derive them from deployment outputs.
Comment on lines
29
to
+33
| "index = 1\n", | ||
| "apim_sku = APIM_SKU.BASICV2 # Options: 'BASICV2', 'STANDARDV2', 'PREMIUMV2'\n", | ||
| "deployment = INFRASTRUCTURE.APIM_ACA # Options: see supported_infras below\n", | ||
| "api_prefix = 'lb-' # ENTER A PREFIX FOR THE APIS TO REDUCE COLLISION POTENTIAL WITH OTHER SAMPLES\n", | ||
| "tags = ['load-balancing'] # ENTER DESCRIPTIVE TAG(S)\n", | ||
| "\n", | ||
| "\n", | ||
| "\n", | ||
| "# ------------------------------\n", | ||
| "# SYSTEM CONFIGURATION\n", | ||
| "# ------------------------------\n", | ||
| "\n", | ||
| "sample_folder = 'load-balancing'\n", | ||
| "rg_name = get_infra_rg_name(deployment, index)\n", | ||
| "supported_infras = [INFRASTRUCTURE.AFD_APIM_PE, INFRASTRUCTURE.APIM_ACA, INFRASTRUCTURE.APPGW_APIM, INFRASTRUCTURE.APPGW_APIM_PE]\n", | ||
| "nb_helper = utils.NotebookHelper(sample_folder, rg_name, rg_location, deployment, supported_infras, index = index, apim_sku = apim_sku)\n", | ||
| "\n", | ||
| "# Define the APIs and their operations and policies\n", | ||
| "\n", | ||
| "# Load and configure backend pool policies\n", | ||
| "pol_aca_backend_pool_load_balancing = utils.read_policy_xml('aca-backend-pool-load-balancing.xml', sample_name = sample_folder)\n", | ||
| "pol_aca_backend_pool_load_balancing_429 = utils.read_policy_xml('aca-backend-pool-load-balancing-with-429.xml', sample_name = sample_folder)\n", | ||
| "pol_aca_backend_pool_prioritized = pol_aca_backend_pool_load_balancing.format(retry_count = 1, backend_id = 'aca-backend-pool-web-api-429-prioritized')\n", | ||
| "pol_aca_backend_pool_prioritized_and_weighted = pol_aca_backend_pool_load_balancing.format(retry_count = 2, backend_id = 'aca-backend-pool-web-api-429-prioritized-and-weighted')\n", | ||
| "pol_aca_backend_pool_weighted_equal = pol_aca_backend_pool_load_balancing.format(retry_count = 1, backend_id = 'aca-backend-pool-web-api-429-weighted-50-50')\n", | ||
| "pol_aca_backend_pool_weighted_unequal = pol_aca_backend_pool_load_balancing.format(retry_count = 1, backend_id = 'aca-backend-pool-web-api-429-weighted-80-20')\n", | ||
| "pol_aca_backend_pool_429_prioritized = pol_aca_backend_pool_load_balancing_429.format(retry_count = 1, backend_id = 'aca-backend-pool-web-api-429-prioritized')\n", | ||
| "\n", | ||
| "# Standard GET operation for all APIs\n", | ||
| "get = GET_APIOperation('This is a standard GET')\n", | ||
| "\n", | ||
| "# API 1: Prioritized backend pool\n", | ||
| "lb_prioritized = API(f'{api_prefix}prioritized-aca-pool', 'Prioritized backend pool', f'/{api_prefix}prioritized', 'This is the API for the prioritized backend pool.', pol_aca_backend_pool_prioritized, [get], tags)\n", | ||
| "# API 2: Prioritized & weighted backend pool\n", | ||
| "lb_prioritized_weighted = API(f'{api_prefix}prioritized-weighted-aca-pool', 'Prioritized & weighted backend pool', f'/{api_prefix}prioritized-weighted', 'This is the API for the prioritized & weighted backend pool.', pol_aca_backend_pool_prioritized_and_weighted, [get], tags)\n", | ||
| "# API 3: Weighted backend pool (equal distribution)\n", | ||
| "lb_equal_weight = API(f'{api_prefix}weighted-equal-aca-pool', 'Weighted backend pool (equal)', f'/{api_prefix}weighted-equal', 'This is the API for the weighted (equal) backend pool.', pol_aca_backend_pool_weighted_equal, [get], tags)\n", | ||
| "# API 4: Weighted backend pool (unequal distribution)\n", | ||
| "lb_unequal_weight = API(f'{api_prefix}weighted-unequal-aca-pool', 'Weighted backend pool (unequal)', f'/{api_prefix}weighted-unequal', 'This is the API for the weighted (unequal) backend pool.', pol_aca_backend_pool_weighted_unequal, [get], tags)\n", | ||
| "# API 5: Prioritized backend pool with 503-to-429 error handling\n", | ||
| "lb_429_prioritized = API(f'{api_prefix}429-prioritized-aca-pool', 'Prioritized backend pool (503→429)', f'/{api_prefix}429-prioritized', 'This is the API for the prioritized backend pool with enhanced error handling that converts 503 to 429.', pol_aca_backend_pool_429_prioritized, [get], tags)\n", | ||
| "\n", | ||
| "# APIs Array\n", | ||
| "apis: List[API] = [lb_prioritized, lb_prioritized_weighted, lb_equal_weight, lb_unequal_weight, lb_429_prioritized]\n", | ||
| "\n", | ||
| "\n", | ||
| "print_ok('Notebook initialized')" | ||
| "tags = ['load-balancing'] # ENTER DESCRIPTIVE TAG(S)" |
There was a problem hiding this comment.
The init cell only contains USER CONFIGURATION, but subsequent cells reference nb_helper, sample_folder, rg_name, and apis. Restore the missing SYSTEM CONFIGURATION section (see samples/_TEMPLATE/create.ipynb) so the notebook can run end-to-end.
Comment on lines
74
to
+90
| @@ -98,6 +81,17 @@ resource apimStorageBlobDataReaderRole 'Microsoft.Authorization/roleAssignments@ | |||
| } | |||
| } | |||
|
|
|||
| // https://learn.microsoft.com/azure/templates/microsoft.authorization/roleassignments | |||
| resource apimStorageBlobDelegatorRole 'Microsoft.Authorization/roleAssignments@2022-04-01' = { | |||
| name: guid(storageAccount.id, apimService.id, 'Storage Blob Delegator') | |||
| scope: storageAccount | |||
| properties: { | |||
| roleDefinitionId: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'db58b8e5-c6ad-4a2a-8342-4190687cbf4a') // Storage Blob Delegator | |||
| principalId: apimService.identity.principalId | |||
There was a problem hiding this comment.
These role assignment resources hard-code role definition GUIDs inline. Consider centralizing role IDs by loading shared/azure-roles.json (as done elsewhere) to avoid duplication/drift and improve readability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.