Skip to content

Commit a495dd7

Browse files
authored
Merge pull request #115 from Azure/fix/align-toolkit-config-format
Fully align filter and override config formats with APIOps Toolkit
2 parents 7d80afb + 9e04d7e commit a495dd7

37 files changed

Lines changed: 2002 additions & 526 deletions

.squad/decisions/decisions.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,53 @@ All new `.ts` files in `src/` and `tests/` must include:
6767
- [Microsoft Open Source Program](https://opensource.microsoft.com/program)
6868

6969
---
70+
71+
### 2026-07-16: Issue #114 Architectural Review — Filter & Override Toolkit Alignment
72+
**By:** ApiOpsLead
73+
**Status:** Approved (Follow-up required)
74+
**What:** Architecture review of issue #114 models, config loader, filter service, override merger, workspace extractor, transitive resolver.
75+
76+
**Key Findings:**
77+
78+
- **OverrideEntry recursive model:** Sound design, well-bounded recursion (max depth 3), no cycle risk.
79+
- **Toolkit parity:** All 14 override sections and 16 filter fields implemented. `Workspace` filter added correctly.
80+
- **Forward compatibility (§VII):** Clean extension points; 1–3 LOC cost per new section/relationship.
81+
- **Override merger traversal:** 🟡 ApiOperationPolicy double-nesting gap — YAML is 3-level (`apis → operations → policies`), but `applyNestedOverride` does 2-level traversal only. Practical impact low (rare use, operation properties typically empty).
82+
- **Filter service sub-filtering:** ✅ Correct semantics (case-insensitive matching, proper empty array handling).
83+
- **Workspace sub-filter consumption:** ✅ Parsed correctly, consumption path can follow in separate PR.
84+
85+
**Verdict:** Approve. Architecture maps cleanly to Toolkit format with sound design and good extension points. ApiOperationPolicy gap is non-blocking — **file follow-up issue.**
86+
87+
**Constitution compliance:** §II (APIM Native), §V (YAGNI), §VI (Testability), §VII (Forward Compatibility)
88+
89+
---
90+
91+
### 2026-07-16: Issue #114 Code Review — Standards & Testability
92+
**By:** CodeReviewer
93+
**Status:** Request Changes (5 required items)
94+
**What:** Standards review of 6 source files, 3 test files, 4 doc/template files for issue #114.
95+
96+
**Required Changes (🟡):**
97+
98+
| ID | Issue | File(s) | Principle |
99+
|----|-------|---------|-----------|
100+
| R1 | ApiOperationPolicy nested override lookup broken | override-merger.ts | §IV Idempotent Operations |
101+
| R2 | Filter name case sensitivity doc/code mismatch | filtering-resources.md | §III Configuration as Code |
102+
| R3 | Duplicate override names silently overwritten | config-loader.ts | §I CLI-First Design |
103+
| R4 | Workspace sub-filters parsed but never consumed | config.ts, config-loader.ts, workspace-extractor.ts | §V Simplicity/YAGNI |
104+
| R5 | Zero test coverage for nested override functionality | override-merger.test.ts, config-loader.test.ts | §VI Testability by Design |
105+
106+
**Details:**
107+
- **R1:** Remove `ApiOperationPolicy` from `CHILD_OVERRIDE_MAP` or implement multi-level traversal for 3-deep nesting.
108+
- **R2:** `matchesFilter()` uses `.toLowerCase()` (case-insensitive); update docs from "case-sensitive" to "case-insensitive."
109+
- **R3:** Emit warning when duplicate `name` in override array; currently silently overwrites.
110+
- **R4:** Either remove workspace sub-filter parsing/model or implement consumption in filter-service/workspace-extractor.
111+
- **R5:** Add tests: ApiDiagnostic nested override, ApiOperation nested override, ProductPolicy nested override, config loader nested override parsing, config loader API sub-filter parsing, config loader workspace sub-filter parsing (if kept).
112+
113+
**Positive Observations:** All 9 files have correct copyright headers. Zero `any` types. All imports use `.js` extensions. Forward compatibility preserved (§VII). Immutability maintained. Secret safety compliant (§VIII). Error handling is actionable. Idempotent design verified (§IV). Legacy alias support with deprecation warnings. Template quality high.
114+
115+
**Verdict:** Well-structured implementation with good constitution compliance. R1–R5 must be resolved before merge. No blockers.
116+
117+
**Constitution compliance:** §I, §III, §IV, §V, §VI, §VII, §VIII
118+
119+
---

docs/ci-cd/azure-devops.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ The extract pipeline pulls configuration from your APIM instance, publishes the
4343

4444
| Parameter | Type | Default | Description |
4545
|-----------|------|---------|-------------|
46-
| `CONFIGURATION_YAML_PATH` | string | `Extract All APIs` | Choose `Extract All APIs` for a full extract, or `configuration.extract.yaml` to use a [filter file](../guides/filtering-resources.md) |
46+
| `CONFIGURATION_YAML_PATH` | string | `Extract All APIs` | Choose `Extract All APIs` for a full extract, or `configuration.extractor.yaml` to use a [filter file](../guides/filtering-resources.md) |
4747
| `resourceGroup` | string | `$(APIM_RESOURCE_GROUP)` | Azure resource group containing your APIM instance |
4848
| `serviceName` | string | `$(APIM_SERVICE_NAME)` | Name of the APIM service instance |
4949

@@ -55,7 +55,7 @@ flowchart TD
5555
B --> C[npm ci]
5656
C --> D{Configuration choice?}
5757
D -->|Extract All APIs| E[apiops extract --resource-group ... --service-name ...]
58-
D -->|configuration.extract.yaml| F[apiops extract ... --filter configuration.extract.yaml]
58+
D -->|configuration.extractor.yaml| F[apiops extract ... --filter configuration.extractor.yaml]
5959
E --> G[Publish pipeline artifact]
6060
F --> G
6161
G --> H[Create branch apim-extract-BuildId]
@@ -92,7 +92,7 @@ The key task is `AzureCLI@2`, which authenticates using your service connection:
9292
--subscription-id $(AZURE_SUBSCRIPTION_ID)
9393
```
9494
95-
When the filter option is selected, `--filter configuration.extract.yaml` is added to the command.
95+
When the filter option is selected, `--filter configuration.extractor.yaml` is added to the command.
9696

9797
> **Why AzureCLI@2?** This task injects Azure credentials into the shell environment, allowing `apiops extract` to authenticate via `DefaultAzureCredential`. See [Authentication Guide](../guides/authentication.md).
9898

docs/ci-cd/github-actions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ The extract workflow pulls configuration from your APIM instance and creates a P
4444
| Input | Description | Options |
4545
|-------|-------------|---------|
4646
| `ENVIRONMENT` | Which APIM instance to extract from | `dev`, `prod` |
47-
| `CONFIGURATION_YAML_PATH` | Extract all APIs or use a filter file | `Extract All APIs`, `configuration.extract.yaml` |
47+
| `CONFIGURATION_YAML_PATH` | Extract all APIs or use a filter file | `Extract All APIs`, `configuration.extractor.yaml` |
4848

4949
### What It Does
5050

docs/commands/extract.md

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ apiops extract \
3434
--subscription-id 00000000-0000-0000-0000-000000000000 \
3535
--resource-group my-rg \
3636
--service-name my-apim \
37-
--filter ./configuration.extract.yaml
37+
--filter ./configuration.extractor.yaml
3838
```
3939

4040
### Extract without transitive dependencies
@@ -98,23 +98,23 @@ By default, `apiops extract` exports **all** resources from the APIM instance (3
9898
To extract only specific resources, pass a YAML filter file with `--filter`:
9999

100100
```yaml
101-
# configuration.extract.yaml
102-
apiNames:
101+
# configuration.extractor.yaml
102+
apis:
103103
- echo-api
104104
- petstore-api
105-
productNames:
105+
products:
106106
- starter
107-
backendNames:
107+
backends:
108108
- backend-api
109-
namedValueNames:
109+
namedValues:
110110
- api-key
111-
tagNames:
111+
tags:
112112
- production
113-
policyFragmentNames:
113+
policyFragments:
114114
- rate-limit-fragment
115-
loggerNames:
115+
loggers:
116116
- appinsights-logger
117-
diagnosticNames:
117+
diagnostics:
118118
- applicationinsights
119119
```
120120
@@ -124,22 +124,22 @@ All 16 supported filter keys:
124124
125125
| Filter key | Resource type |
126126
|------------|---------------|
127-
| `apiNames` | APIs |
128-
| `backendNames` | Backends |
129-
| `productNames` | Products |
130-
| `namedValueNames` | Named values |
131-
| `loggerNames` | Loggers |
132-
| `diagnosticNames` | Diagnostics |
133-
| `tagNames` | Tags |
134-
| `policyFragmentNames` | Policy fragments |
135-
| `gatewayNames` | Gateways |
136-
| `versionSetNames` | API version sets |
137-
| `groupNames` | Groups |
138-
| `subscriptionNames` | Subscriptions |
139-
| `schemaNames` | Schemas |
140-
| `policyRestrictionNames` | Policy restrictions |
141-
| `documentationNames` | Documentation resources |
142-
| `workspaceNames` | Workspaces |
127+
| `apis` | APIs |
128+
| `backends` | Backends |
129+
| `products` | Products |
130+
| `namedValues` | Named values |
131+
| `loggers` | Loggers |
132+
| `diagnostics` | Diagnostics |
133+
| `tags` | Tags |
134+
| `policyFragments` | Policy fragments |
135+
| `gateways` | Gateways |
136+
| `versionSets` | API version sets |
137+
| `groups` | Groups |
138+
| `subscriptions` | Subscriptions |
139+
| `schemas` | Schemas |
140+
| `policyRestrictions` | Policy restrictions |
141+
| `documentations` | Documentation resources |
142+
| `workspaces` | Workspaces |
143143

144144
### Transitive dependencies
145145

docs/commands/init.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,26 +78,27 @@ In interactive mode (the default when running in a terminal), `apiops init` prom
7878

7979
| File | Purpose |
8080
|------|---------|
81-
| `.github/workflows/extract.yaml` | Pipeline to extract APIM artifacts |
82-
| `.github/workflows/publish.yaml` | Pipeline to publish artifacts to APIM |
83-
| `configuration.extract.yaml` | Sample filter configuration for extraction |
81+
| `.github/workflows/run-apim-extractor.yml` | Workflow to extract APIM artifacts |
82+
| `.github/workflows/run-apim-publisher.yml` | Workflow to publish artifacts to APIM |
83+
| `configuration.extractor.yaml` | Sample filter configuration for extraction |
8484
| `configuration.{env}.yaml` | Override templates per environment (e.g., `configuration.dev.yaml`, `configuration.prod.yaml`) |
8585
| `IDENTITY-SETUP-GITHUB.md` | Step-by-step guide for configuring federated credentials |
8686

8787
### Azure DevOps (`--ci azure-devops`)
8888

8989
| File | Purpose |
9090
|------|---------|
91-
| `pipelines/extract.yaml` | Pipeline to extract APIM artifacts |
92-
| `pipelines/publish.yaml` | Pipeline to publish artifacts to APIM |
93-
| `configuration.extract.yaml` | Sample filter configuration for extraction |
91+
| `.azdo/pipelines/run-apim-extractor.yml` | Pipeline to extract APIM artifacts |
92+
| `.azdo/pipelines/run-apim-publisher.yml` | Pipeline to publish artifacts to APIM |
93+
| `configuration.extractor.yaml` | Sample filter configuration for extraction |
9494
| `configuration.{env}.yaml` | Override templates per environment |
9595
| `IDENTITY-SETUP-AZDO.md` | Step-by-step guide for configuring service connections |
9696

9797
### Both platforms
9898

9999
| File | Purpose |
100100
|------|---------|
101+
| `.github/prompts/apiops-setup-identity.prompt.md` | Copilot prompt for identity setup |
101102
| `<artifact-dir>/` | Empty artifact directory (default: `./apim-artifacts`) |
102103

103104
## Package consumption modes

0 commit comments

Comments
 (0)