feat(245): external-name ADR compliance for ServiceCredentialBinding#306
Open
1122Louis wants to merge 8 commits into
Open
feat(245): external-name ADR compliance for ServiceCredentialBinding#3061122Louis wants to merge 8 commits into
1122Louis wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and controller behavior to ensure ServiceCredentialBinding external-names are UUID/GUID-based and remain stable across upgrade/import flows.
Changes:
- Introduces upgrade + e2e tests and CR testdata for ServiceCredentialBinding external-name/import scenarios
- Adds adoption behavior to rewrite non-GUID external-names to the discovered binding GUID during Observe
- Tightens Update/Delete logic to act only when external-name is a valid UUID, plus new unit tests for these paths
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/upgrade/testdata/customCrs/externalNames/serviceCredentialBinding/service_instance.yaml | Adds ServiceInstance CR used by upgrade external-name test scenario |
| test/upgrade/testdata/customCrs/externalNames/serviceCredentialBinding/service_credential_binding.yaml | Adds SCB CR used by upgrade external-name test scenario |
| test/upgrade/service_credential_binding_external_name_upgrade_test.go | New upgrade test asserting external-name UUID format and stability across upgrade |
| test/e2e/crs/externalNamesImport/serviceCredentialBinding/serviceinstance.yaml | Adds dependent ServiceInstance CR for e2e import flow |
| test/e2e/crs/externalNamesImport/serviceCredentialBinding/import.yaml | Adds observed Org/Space CRs used by import test resources |
| test/e2e/cloudfoundry_servicecredentialbinding_import_test.go | New e2e test exercising SCB import flow |
| internal/controller/servicecredentialbinding/controller_test.go | Adds unit tests for invalid external-name UUID handling and adoption behavior |
| internal/controller/servicecredentialbinding/controller.go | Adds UUID validation + adoption update of external-name; changes Update/Delete behavior |
| apis/resources/v1alpha1/servicecredentialbinding_types.go | Documents external-name expectations and how to discover GUID |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+251
to
+258
| externalName := meta.GetExternalName(cr) | ||
| if uuid.Validate(externalName) != nil { | ||
| // External name is not a valid UUID; nothing to delete in CF | ||
| return managed.ExternalDelete{}, nil | ||
| } | ||
|
|
||
| err := scb.Delete(ctx, c.scbClient, externalName) | ||
| if err := clients.IgnoreNotFoundErr(err); err != nil { |
| errUpdateStatus = "cannot update status after retiring binding: %w" | ||
| errExtractParams = "cannot extract specified parameters: %w" | ||
| errUnknownState = "unknown last operation state for " + resourceType + " in " + externalSystem | ||
| errUpdateCR = "cannot update managed resource" |
| if guid != serviceBinding.GUID { | ||
| meta.SetExternalName(cr, serviceBinding.GUID) | ||
| if err := c.kube.Update(ctx, cr); err != nil { | ||
| return managed.ExternalObservation{}, fmt.Errorf("%s: %w", errUpdateCR, err) |
Comment on lines
+75
to
+79
| func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { | ||
| serviceCredentialBinding := &v1alpha1.ServiceCredentialBinding{} | ||
| r := cfg.Client().Resources() | ||
|
|
||
| err := r.Get(ctx, serviceCredentialBindingName, cfg.Namespace(), serviceCredentialBinding) |
Collaborator
There was a problem hiding this comment.
We could have both register, or extract a helper. The post-upgrade r.Get may silently fail or use the wrong kind if the scheme isn't registered.
| t.Fatalf("External name '%s' does not match expected UUID format", externalName) | ||
| } | ||
|
|
||
| return context.WithValue(ctx, "preUpgradeExternalName", externalName) |
Collaborator
There was a problem hiding this comment.
Should be Must be an unexported struct type:
type contextKey struct{}
type preUpgradeExternalNameKey struct{}
// then
context.WithValue(ctx, preUpgradeExternalNameKey{}, externalName)
ctx.Value(preUpgradeExternalNameKey{}).(string)
| t.Fatalf("External name '%s' does not match expected UUID format after upgrade", externalName) | ||
| } | ||
|
|
||
| preUpgradeExternalName, ok := ctx.Value("preUpgradeExternalName").(string) |
Comment on lines
+116
to
+121
| // External-Name Configuration: | ||
| // - Follows Standard: yes | ||
| // - Format: Service Credential Binding GUID (UUID format) | ||
| // - How to find: | ||
| // - UI: Not available in the BTP Cockpit | ||
| // - CLI: Use CF CLI: `cf service-keys <SERVICE_INSTANCE>` and look up the key GUID via `cf curl /v3/service_credential_bindings?names=<KEY_NAME>` |
- Observe(): adopt existing resources by updating external-name to real GUID - Update(): skip CF update if external-name is not a valid UUID - Delete(): use external-name GUID directly + ignore NotFound errors - Add External-Name Configuration doc comment to types file - Add unit tests: adoption, invalid UUID format, not-found on delete - Add E2E import test using ImportTester pattern - Add upgrade test verifying external-name survives provider upgrade Closes SAP#245
1122Louis
force-pushed
the
feature/245-servicecredentialbinding-external-name-adr
branch
from
e0073e0 to
82ad561
Compare
- Extract isBindingNotFoundError() to reduce Observe() cyclomatic complexity - Extract isValidUUID() helper to avoid nilerr lint false positive - Regenerate CRDs
SatabdiG
reviewed
Jun 8, 2026
SatabdiG
left a comment
SatabdiG
left a comment
Collaborator
There was a problem hiding this comment.
Really good work! some comments from my side but overall solid
Comment on lines
+116
to
+121
| // External-Name Configuration: | ||
| // - Follows Standard: yes | ||
| // - Format: Service Credential Binding GUID (UUID format) | ||
| // - How to find: | ||
| // - UI: Not available in the BTP Cockpit | ||
| // - CLI: Use CF CLI: `cf service-keys <SERVICE_INSTANCE>` and look up the key GUID via `cf curl /v3/service_credential_bindings?names=<KEY_NAME>` |
| t.Fatalf("External name '%s' does not match expected UUID format", externalName) | ||
| } | ||
|
|
||
| return context.WithValue(ctx, "preUpgradeExternalName", externalName) |
Collaborator
There was a problem hiding this comment.
Should be Must be an unexported struct type:
type contextKey struct{}
type preUpgradeExternalNameKey struct{}
// then
context.WithValue(ctx, preUpgradeExternalNameKey{}, externalName)
ctx.Value(preUpgradeExternalNameKey{}).(string)
| } | ||
|
|
||
| annotations := serviceCredentialBinding.GetAnnotations() | ||
| externalName, exists := annotations["crossplane.io/external-name"] |
Collaborator
There was a problem hiding this comment.
we should use meta.GetExternalName(serviceCredentialBinding)
Comment on lines
+75
to
+79
| func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { | ||
| serviceCredentialBinding := &v1alpha1.ServiceCredentialBinding{} | ||
| r := cfg.Client().Resources() | ||
|
|
||
| err := r.Get(ctx, serviceCredentialBindingName, cfg.Namespace(), serviceCredentialBinding) |
Collaborator
There was a problem hiding this comment.
We could have both register, or extract a helper. The post-upgrade r.Get may silently fail or use the wrong kind if the scheme isn't registered.
1122Louis
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Failure
1122Louis
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Failure
1122Louis
temporarily deployed
to
pr-e2e-no-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Failure
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-no-approval
GitHub Actions
Inactive
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
1122Louis
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Failure
1122Louis
temporarily deployed
to
pr-e2e-no-approval
GitHub Actions
Inactive
- Fix doc comment formatting (proper indentation) - Use unexported struct for context key (avoid collisions) - Use meta.GetExternalName() instead of direct annotation access - Register scheme in post-upgrade assessment - Improve Delete logic to fallback to status GUID (prevent orphaning)
1122Louis
temporarily deployed
to
pr-e2e-approval
GitHub Actions
Inactive
- Remove extra blank line in apis/generate.go - Add missing blank line after comment in orgquota_types.go - Fix comment indentation in serviceinstance_types.go
- Update InvalidUUID_SkipsDelete test to expect Delete call with status GUID - Add InvalidUUID_BothInvalid_SkipsDelete test for true skip case - Reflects new Delete logic that falls back to status GUID when external-name is not a valid UUID
Use standard indentation for doc comment (matching other types in codebase)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Testing
Closes #245