Skip to content

Conversation

@Hubert-Szczepanski-SAP
Copy link
Contributor

@Hubert-Szczepanski-SAP Hubert-Szczepanski-SAP commented Oct 20, 2025

@andreaskienle andreaskienle requested a review from Copilot October 27, 2025 09:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in provider mapping by refactoring how provider types are resolved. Instead of looking up provider configurations by name and checking apiVersion patterns, it now directly extracts and normalizes the domain from the apiVersion string.

Key changes:

  • Replaced resolveProviderType function with resolveProviderTypeFromApiVersion that extracts domain from apiVersion
  • Removed dependency on providerConfigsList parameter in the resolution logic
  • Updated test suite to reflect the new function signature and behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/components/Graphs/graphUtils.ts Refactored provider type resolution to extract domain directly from apiVersion instead of iterating through provider configs
src/components/Graphs/graphUtils.spec.ts Updated tests to validate domain extraction and "account." prefix removal logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
export const resolveProviderTypeFromApiVersion = (apiVersion: string): string => {
// Extract domain from apiVersion (e.g. "account.btp.sap.crossplane.io/v1alpha1" -> "account.btp.sap.crossplane.io")
const domain = apiVersion.split('/')[0] || '';
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback to empty string is ineffective because split('/')[0] will never return undefined or null. If apiVersion is an empty string, split('/')[0] returns an empty string. If apiVersion is undefined or null, this will throw an error. Consider adding a guard: const domain = apiVersion?.split('/')[0] || ''; or handle the undefined/null case before splitting.

Suggested change
const domain = apiVersion.split('/')[0] || '';
const domain = apiVersion?.split('/')[0] || '';

Copilot uses AI. Check for mistakes.
andreaskienle
andreaskienle previously approved these changes Oct 27, 2025
Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out Copilot's comment. I think it makes sense to safeguard against null/undefined being passed even though we have TypeScript. It's just a small change and an extra test.

Apart from that: 👍🏻

@Hubert-Szczepanski-SAP Hubert-Szczepanski-SAP merged commit 9ec55c2 into main Oct 29, 2025
5 checks passed
@Hubert-Szczepanski-SAP Hubert-Szczepanski-SAP deleted the fix/mapping-providers-by-api-version branch October 29, 2025 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Change the way how we are matching provider name with managed resource

4 participants