🐛 Add enabled property to llm proxy client#1374
Conversation
Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com>
📝 WalkthroughWalkthroughThese changes address the issue where the LLM proxy client was attempting connections despite being disabled by operators. An Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
Adding release-0.4 cherry-pick label. Remove if I'm wrong please. |
Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml (1)
1-3: Consider fixing the changelog filename typo (enavled→enabled).The content is fine, but correcting the filename spelling will keep changelog entries easier to scan and search.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml` around lines 1 - 3, The changelog filename contains a typo ("enavled"); rename the file from 1374-add-enavled-property-to-llmproxy-client.yaml to 1374-add-enabled-property-to-llmproxy-client.yaml and update any references (CI, release tooling, or changelog indexes) that mention the old filename so searches and automated tooling use the corrected "enabled" spelling.vscode/core/src/clients/ProfileSyncClient.ts (1)
182-200: Add focused tests forenablednormalization and fallback behavior.Please add coverage for at least:
enabled: false,enabled: "false", and missingenabled(defaults to true) to prevent regressions in proxy routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/clients/ProfileSyncClient.ts` around lines 182 - 200, Add unit tests for ProfileSyncClient that exercise the LLM proxy config normalization branch where rawConfig may be a string or object: specifically test when enabled is false (boolean), enabled is "false" (string), and when enabled is omitted (should default to true). Instantiate or call the method that parses rawConfig (the code path that produces config and sets this.llmProxyConfig / logs, i.e., the LLM proxy config fetch/normalize logic in ProfileSyncClient) with each variant and assert the resulting this.llmProxyConfig.available flag and endpoint, plus that logs or returned config.model/ enabled match expectations to ensure proxy routing/fallback behaves correctly. Make tests cover both stringified JSON input and object input to match the existing typeof rawConfig === "string" branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml`:
- Around line 1-3: The changelog filename contains a typo ("enavled"); rename
the file from 1374-add-enavled-property-to-llmproxy-client.yaml to
1374-add-enabled-property-to-llmproxy-client.yaml and update any references (CI,
release tooling, or changelog indexes) that mention the old filename so searches
and automated tooling use the corrected "enabled" spelling.
In `@vscode/core/src/clients/ProfileSyncClient.ts`:
- Around line 182-200: Add unit tests for ProfileSyncClient that exercise the
LLM proxy config normalization branch where rawConfig may be a string or object:
specifically test when enabled is false (boolean), enabled is "false" (string),
and when enabled is omitted (should default to true). Instantiate or call the
method that parses rawConfig (the code path that produces config and sets
this.llmProxyConfig / logs, i.e., the LLM proxy config fetch/normalize logic in
ProfileSyncClient) with each variant and assert the resulting
this.llmProxyConfig.available flag and endpoint, plus that logs or returned
config.model/ enabled match expectations to ensure proxy routing/fallback
behaves correctly. Make tests cover both stringified JSON input and object input
to match the existing typeof rawConfig === "string" branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67f43eb4-157a-46cb-9410-f0a9afdbd842
📒 Files selected for processing (2)
changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yamlvscode/core/src/clients/ProfileSyncClient.ts
|
PR cherry-picked to branch release-0.4. Backport PR: #1378 |
Resolves #1365 Operator PR with new configmap property: konveyor/operator#558 If the `enabled` property is not present in the configmap, it is assumed to be true to ensure compatibility with older versions of the operator. To prevent the bug from occurring, both the operator and the extension will need to be updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * The LLM proxy client now supports an `enabled` configuration setting to control whether the proxy is active in your environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Co-authored-by: Alejandro Brugarolas <117646518+abrugaro@users.noreply.github.com>
Resolves konveyor#1365 Operator PR with new configmap property: konveyor/operator#558 If the `enabled` property is not present in the configmap, it is assumed to be true to ensure compatibility with older versions of the operator. To prevent the bug from occurring, both the operator and the extension will need to be updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * The LLM proxy client now supports an `enabled` configuration setting to control whether the proxy is active in your environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Resolves konveyor#1365 Operator PR with new configmap property: konveyor/operator#558 If the `enabled` property is not present in the configmap, it is assumed to be true to ensure compatibility with older versions of the operator. To prevent the bug from occurring, both the operator and the extension will need to be updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * The LLM proxy client now supports an `enabled` configuration setting to control whether the proxy is active in your environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Resolves konveyor#1365 Operator PR with new configmap property: konveyor/operator#558 If the `enabled` property is not present in the configmap, it is assumed to be true to ensure compatibility with older versions of the operator. To prevent the bug from occurring, both the operator and the extension will need to be updated <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * The LLM proxy client now supports an `enabled` configuration setting to control whether the proxy is active in your environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
Resolves #1365
Operator PR with new configmap property: konveyor/operator#558
If the
enabledproperty is not present in the configmap, it is assumed to be true to ensure compatibility with older versions of the operator. To prevent the bug from occurring, both the operator and the extension will need to be updatedSummary by CodeRabbit
enabledconfiguration setting to control whether the proxy is active in your environment.