Edit MCP - Crossplane providers: Handle prefix correctly and do not drop unknown providers #398
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the hook that handles Crossplane provider selection to correctly handle provider naming prefixes and prevent dropping unknown providers. The changes address the issue where providers can be stored in the API without the provider- prefix (e.g., btp) while the component list includes them with the prefix (e.g., provider-btp).
Key changes:
- Added
originalNamefield to track the provider name as stored in the API - Refactored provider matching logic to handle both prefixed and non-prefixed provider names
- Preserved unknown/custom providers that aren't in the available components list
- Extracted complex logic into well-tested helper functions for better maintainability
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/lib/api/types/crate/createManagedControlPlane.ts |
Added originalName field to ComponentsListItem interface and updated provider mapping to use it for API requests |
src/components/Wizards/CreateManagedControlPlane/useComponentsSelectionData.ts |
Refactored the hook by extracting helper functions for provider prefix handling, component mapping, validation, and custom provider addition |
src/components/Wizards/CreateManagedControlPlane/useComponentsSelectionData.spec.ts |
Added comprehensive test coverage for all new helper functions and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| export const isProviderComponent = (name: string): boolean => { | ||
| return name.includes('provider') && name !== 'crossplane'; |
There was a problem hiding this comment.
The isProviderComponent function uses name.includes('provider') which will match any name containing the substring "provider", not just those starting with "provider-". This could incorrectly classify components like "my-provider-tool" or "service-provider" as providers. Consider using a more precise check like name.startsWith('provider-') to avoid false positives.
| return name.includes('provider') && name !== 'crossplane'; | |
| return name.startsWith('provider-') && name !== 'crossplane'; |
| describe('mapToComponentsListItem', () => { | ||
| it('maps component with sorted versions and applies initial selection', () => { | ||
| const item = createManagedComponent('crossplane', ['1.15.0', '1.20.1', '1.19.0']); | ||
| const initialSelection: InitialSelection = { | ||
| crossplane: { isSelected: true, version: '1.19.0' }, | ||
| }; | ||
|
|
||
| const result = mapToComponentsListItem(item, initialSelection, undefined); | ||
|
|
||
| expect(result.versions).toEqual(['1.20.1', '1.19.0', '1.15.0']); | ||
| expect(result.selectedVersion).toBe('1.19.0'); | ||
| expect(result.isSelected).toBe(true); | ||
| expect(result.isProvider).toBe(false); | ||
| }); | ||
|
|
||
| it('adds initial selection version to versions array if not present', () => { | ||
| const item = createManagedComponent('crossplane', ['1.15.0', '1.19.0']); | ||
| const initialSelection: InitialSelection = { | ||
| crossplane: { isSelected: true, version: '1.20.1' }, | ||
| }; | ||
|
|
||
| const result = mapToComponentsListItem(item, initialSelection, undefined); | ||
|
|
||
| expect(result.versions).toContain('1.20.1'); | ||
| expect(result.versions).toEqual(['1.20.1', '1.19.0', '1.15.0']); | ||
| expect(result.selectedVersion).toBe('1.20.1'); | ||
| }); | ||
|
|
||
| it('applies template default when no initial selection', () => { | ||
| const item = createManagedComponent('flux', ['2.15.0', '2.16.2']); | ||
| const template = createTemplate([{ name: 'flux', version: '2.15.0' }]); | ||
|
|
||
| const result = mapToComponentsListItem(item, undefined, template); | ||
|
|
||
| expect(result.selectedVersion).toBe('2.15.0'); | ||
| expect(result.isSelected).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests for mapToComponentsListItem don't include a test case that verifies the originalName field is correctly set for provider components. Consider adding a test that checks a provider component (e.g., provider-btp) with initial selection stored as btp correctly sets originalName to btp and name to provider-btp. This would help ensure the prefix handling logic works correctly.
| }; | ||
|
|
||
| export const isProviderComponent = (name: string): boolean => { | ||
| return name.includes('provider') && name !== 'crossplane'; |
There was a problem hiding this comment.
The isProviderComponent function will incorrectly return true for a component literally named "provider" (without any suffix), treating it as a Crossplane provider when it might not be. While this is an edge case, consider whether such a component name is possible and if it needs special handling.
| return name.includes('provider') && name !== 'crossplane'; | |
| return name.includes('provider') && name !== 'crossplane' && name !== 'provider'; |
What this PR does / why we need it:
Edit MCP - Crossplane providers: Handle prefix correctly and do not drop unknown providers. Refactored hook that handles it