fix(cli): enforce exact label selector matching#418
Conversation
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI/server label-selector behavior to require exact label key matching by filtering configurations (instead of mutating resources to “fill” labels), and tightens CLI parsing to key=value (comma-separated) only.
Changes:
- Replace
fillLabelsusage withfilterConfigurationwhen loading local config (CLI task) and when preparing local config in the sync server handler. - Add
parseLabelSelector(Commander arg parser) to enforcekey=valueparsing only. - Add/extend tests to verify exact label key matching and selector parsing validation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/cli/src/tasks/load_local.ts | Switch local label-selector handling from label injection to config filtering. |
| apps/cli/src/tasks/load_local.spec.ts | Add integration-style test ensuring local load filters by exact label key. |
| apps/cli/src/server/sync.ts | Filter local config by label-selector before lint/diff; remove fillLabels. |
| apps/cli/src/command/utils.spec.ts | Add unit tests for exact key-match filtering and strict selector parsing. |
| apps/cli/src/command/helper.ts | Replace qs parsing with strict key=value label selector parser. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/src/tasks/load_local.spec.ts (1)
10-49: LGTM!The test properly validates the new exact label key matching behavior with appropriate setup/teardown using
try/finally. The test case clearly demonstrates that resources with different label keys (e.g.,appnamevsname) are correctly filtered out.Consider adding tests for edge cases:
- Empty label selector (should return all resources)
- Multiple label key requirements (all must match)
- Resources without any labels (should be filtered out)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/tasks/load_local.spec.ts` around lines 10 - 49, Add three additional unit tests in load_local.spec.ts alongside the existing "filters local resources by exact label key match" test to cover edge cases: (1) an "empty label selector" test that calls LoadLocalConfigurationTask with an empty selector (e.g., {}) and asserts ctx.local.services returns all services from the adc.yaml, (2) a "multiple label key requirements" test that supplies a selector with two keys (e.g., { name: 'x', env: 'prod' }) and asserts only resources matching both labels are returned by LoadLocalConfigurationTask, and (3) a "resources without any labels" test that includes a service without a labels field in the yaml and asserts it is filtered out by LoadLocalConfigurationTask; reuse the mkdtempSync/writeFileSync/try...finally cleanup pattern and assert against ctx.local.services as in the existing spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/server/sync.ts`:
- Around line 59-62: The call site in sync.ts unconditionally invokes
filterConfiguration on local with task.opts.labelSelector; add the same guard
used in load_local.ts so you only call filterConfiguration when
task.opts.labelSelector is set/non-empty (e.g., check task.opts.labelSelector
truthiness or length) before assigning [local] = filterConfiguration(...);
update both places in sync.ts where filterConfiguration is used to match the
guarded pattern and ensure local remains unchanged if the selector is absent.
---
Nitpick comments:
In `@apps/cli/src/tasks/load_local.spec.ts`:
- Around line 10-49: Add three additional unit tests in load_local.spec.ts
alongside the existing "filters local resources by exact label key match" test
to cover edge cases: (1) an "empty label selector" test that calls
LoadLocalConfigurationTask with an empty selector (e.g., {}) and asserts
ctx.local.services returns all services from the adc.yaml, (2) a "multiple label
key requirements" test that supplies a selector with two keys (e.g., { name:
'x', env: 'prod' }) and asserts only resources matching both labels are returned
by LoadLocalConfigurationTask, and (3) a "resources without any labels" test
that includes a service without a labels field in the yaml and asserts it is
filtered out by LoadLocalConfigurationTask; reuse the
mkdtempSync/writeFileSync/try...finally cleanup pattern and assert against
ctx.local.services as in the existing spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4cf46dc-c944-45c8-8773-c8b6dc9629a9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/cli/package.jsonapps/cli/src/command/helper.tsapps/cli/src/command/utils.spec.tsapps/cli/src/server/sync.tsapps/cli/src/tasks/load_local.spec.tsapps/cli/src/tasks/load_local.ts
💤 Files with no reviewable changes (1)
- apps/cli/package.json
| [local] = filterConfiguration( | ||
| local, | ||
| task.opts.labelSelector, | ||
| ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if filterConfiguration handles undefined/empty rules gracefully
ast-grep --pattern $'export const filterConfiguration = ($_, $_, $_) => {
$$$
}'Repository: api7/adc
Length of output: 34
🏁 Script executed:
# Find filterConfiguration function definition
rg 'export.*filterConfiguration\s*=' -A 10Repository: api7/adc
Length of output: 900
🏁 Script executed:
# Read sync.ts file around the flagged lines
cat -n apps/cli/src/server/sync.ts | sed -n '50,115p'Repository: api7/adc
Length of output: 2463
🏁 Script executed:
# Find load_local.ts and check the guard pattern
find . -name 'load_local.ts' -type fRepository: api7/adc
Length of output: 86
🏁 Script executed:
# Get full filterConfiguration implementation
cat -n apps/cli/src/command/utils.ts | grep -A 30 'export const filterConfiguration'Repository: api7/adc
Length of output: 1221
🏁 Script executed:
# Check the labelFilter function definition and implementation
rg 'export.*labelFilter\s*=|function labelFilter' -A 15Repository: api7/adc
Length of output: 34
🏁 Script executed:
# Read load_local.ts to see the guard pattern mentioned in the review
cat -n apps/cli/src/tasks/load_local.tsRepository: api7/adc
Length of output: 4301
Add guards before calling filterConfiguration for consistency with load_local.ts.
The code unconditionally calls filterConfiguration even when task.opts.labelSelector is undefined or empty. While labelFilter has a default value (rules = {}), this results in unnecessary iteration and is inconsistent with the guard pattern in load_local.ts (lines 101-104). Add the same guard to both calls in sync.ts.
Suggested fix
- [local] = filterConfiguration(
- local,
- task.opts.labelSelector,
- ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
+ if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) {
+ [local] = filterConfiguration(
+ local,
+ task.opts.labelSelector,
+ ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
+ }Apply the same guard to line 107:
- [remote] = filterConfiguration(remote, task.opts.labelSelector);
+ if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) {
+ [remote] = filterConfiguration(remote, task.opts.labelSelector);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [local] = filterConfiguration( | |
| local, | |
| task.opts.labelSelector, | |
| ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration]; | |
| if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) { | |
| [local] = filterConfiguration( | |
| local, | |
| task.opts.labelSelector, | |
| ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/server/sync.ts` around lines 59 - 62, The call site in sync.ts
unconditionally invokes filterConfiguration on local with
task.opts.labelSelector; add the same guard used in load_local.ts so you only
call filterConfiguration when task.opts.labelSelector is set/non-empty (e.g.,
check task.opts.labelSelector truthiness or length) before assigning [local] =
filterConfiguration(...); update both places in sync.ts where
filterConfiguration is used to match the guarded pattern and ensure local
remains unchanged if the selector is absent.
| task: async () => { | ||
| // Merge label selectors from CLI inputs to each resource | ||
| fillLabels(ctx.local, labelSelector); | ||
| [ctx.local] = filterConfiguration(ctx.local, labelSelector); |
There was a problem hiding this comment.
I don't understand what you're trying to do. The original design was intentional—label filtering always occurs remotely. For local resources, the ADC will only populate the label fields you want to filter, not perform the filtering itself.
I believe this stance remains unchanged.
If the label filter fails to accurately search and filter resources remotely, you should improve the specific backend and require the backend API implementation to ensure its filters are precise rather than fuzzy.
| services: [ | ||
| { | ||
| name: 'match-by-name', | ||
| labels: { name: 'yanglao_wx_pgm' }, |
There was a problem hiding this comment.
What does this name signify? Who are you representing in submitting this PR?
Please ensure you always use neutral, non-specific terminology for testing purposes.
There was a problem hiding this comment.
You're right. I'll replace yanglao_wx_pgm with neutral placeholder values (e.g., test-service, foo) across all test cases.
| verbose: number; | ||
| } | ||
|
|
||
| export const parseLabelSelector = ( |
There was a problem hiding this comment.
What is the difference from the old implementation based on qs?
There was a problem hiding this comment.
The main difference is that qs supports nested object notation—for example, a[b]=c is parsed as { a: { b: 'c' } } rather than { 'a[b]': 'c' }.
If a label value happens to contain [ or ] characters, qs would produce
an unexpected nested structure instead of a flat string value.
|
After discussing with @bzp2010 This is the expected behavior. Each label selector item is written to the local configuration and synchronized to the gateway. The input accepted by ADC is your desired state for the remote configuration; what you input is what the remote state should be. If you expect a certain resource not to be synchronized from local to remote, you should not include it as input during the sync process. Auto-filling filtered tag keys and values ensures that when synchronizing again using the same tag filters as the current setup, the scope of covered resources remains consistent. |
Changes
parseLabelSelectorutility with strictkey=valuevalidationfillLabels()withfilterConfiguration()insync.tsandload_local.tsqsand@types/qsdependenciesChecklist
Summary by CodeRabbit
Release Notes
Refactor
Tests