Skip to content

Commit 038fc6d

Browse files
fix: update subscription.go with external name handling for observe i… (#486)
* fix: update subscription.go with external name handling for observe imports * chore: add test for new changes * fix: update code to be complain to external-name adr --------- Co-authored-by: sdischer-sap <stephan.discher@sap.com>
1 parent 4bd9267 commit 038fc6d

2 files changed

Lines changed: 109 additions & 0 deletions

File tree

internal/controller/account/subscription/subscription.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,20 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
129129
if !ok {
130130
return managed.ExternalObservation{}, errors.New(errNotSubscription)
131131
}
132+
externalName := meta.GetExternalName(cr)
133+
if externalName == cr.Name {
134+
// Set correct external-name format for import/observe scenarios
135+
if isObserveOnly(cr) && cr.Spec.ForProvider.AppName != "" && cr.Spec.ForProvider.PlanName != "" {
136+
expectedExternalName := fmt.Sprintf("%s/%s", cr.Spec.ForProvider.AppName, cr.Spec.ForProvider.PlanName)
137+
return managed.ExternalObservation{}, errors.Errorf(
138+
"For Observe-only Subscriptions, external-name must be set to 'appName/planName' format. "+
139+
"Found: '%s'. Expected: '%s'. "+
140+
"Please set the annotation: crossplane.io/external-name: \"%s\"",
141+
externalName, expectedExternalName, expectedExternalName,
142+
)
143+
144+
}
145+
}
132146

133147
apiRes, err := c.loadSubscription(ctx, cr)
134148
if err != nil {
@@ -260,3 +274,18 @@ func (c *external) shouldRecreateOnFailure(cr *v1alpha1.Subscription, apiRes *su
260274
}
261275
return false
262276
}
277+
278+
func isObserveOnly(cr *v1alpha1.Subscription) bool {
279+
if len(cr.Spec.ManagementPolicies) == 0 {
280+
return false // default is full management
281+
}
282+
for _, policy := range cr.Spec.ManagementPolicies {
283+
if policy == xpv1.ManagementActionCreate {
284+
return false // allowed to create
285+
}
286+
if policy == xpv1.ManagementActionAll {
287+
return false // allowed to create
288+
}
289+
}
290+
return true // only Observe/Update/Delete, no Create
291+
}

internal/controller/account/subscription/subscription_test.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,35 @@ func TestObserve(t *testing.T) {
167167
}), WithExternalName("name1/plan2")),
168168
},
169169
},
170+
"ObserveOnly_RequiresCorrectExternalNameFormat": {
171+
reason: "Observe-only resource with default external-name should return validation error",
172+
args: args{
173+
cr: NewSubscription("test-subscription",
174+
WithData(v1alpha1.SubscriptionSpec{
175+
ForProvider: v1alpha1.SubscriptionParameters{
176+
AppName: "sapappstudio",
177+
PlanName: "standard-edition",
178+
},
179+
}),
180+
WithManagementPolicies(xpv1.ManagementActionObserve),
181+
WithExternalName("test-subscription"), // Wrong - equals metadata.name
182+
),
183+
},
184+
want: want{
185+
o: managed.ExternalObservation{},
186+
err: errors.New("For Observe-only Subscriptions, external-name must be set to 'appName/planName' format. Found: 'test-subscription'. Expected: 'sapappstudio/standard-edition'. Please set the annotation: crossplane.io/external-name: \"sapappstudio/standard-edition\""),
187+
cr: NewSubscription("test-subscription",
188+
WithData(v1alpha1.SubscriptionSpec{
189+
ForProvider: v1alpha1.SubscriptionParameters{
190+
AppName: "sapappstudio",
191+
PlanName: "standard-edition",
192+
},
193+
}),
194+
WithManagementPolicies(xpv1.ManagementActionObserve),
195+
WithExternalName("test-subscription"),
196+
),
197+
},
198+
},
170199
}
171200
for name, tc := range tests {
172201
t.Run(name, func(t *testing.T) {
@@ -681,3 +710,54 @@ func WithRecreateOnSubscriptionFailure() SubscriptionModifier {
681710
r.Spec.RecreateOnSubscriptionFailure = true
682711
}
683712
}
713+
714+
func TestIsObserveOnly(t *testing.T) {
715+
tests := map[string]struct {
716+
managementPolicies []xpv1.ManagementAction
717+
expected bool
718+
}{
719+
"ObserveOnly": {
720+
managementPolicies: []xpv1.ManagementAction{xpv1.ManagementActionObserve},
721+
expected: true,
722+
},
723+
"AllActions": {
724+
managementPolicies: []xpv1.ManagementAction{xpv1.ManagementActionAll},
725+
expected: false,
726+
},
727+
"IncludesCreate": {
728+
managementPolicies: []xpv1.ManagementAction{xpv1.ManagementActionObserve, xpv1.ManagementActionCreate},
729+
expected: false,
730+
},
731+
"ObserveAndUpdate": {
732+
managementPolicies: []xpv1.ManagementAction{xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate},
733+
expected: true,
734+
},
735+
"EmptyDefault": {
736+
managementPolicies: []xpv1.ManagementAction{},
737+
expected: false,
738+
},
739+
"NilDefault": {
740+
managementPolicies: nil,
741+
expected: false,
742+
},
743+
}
744+
745+
for name, tc := range tests {
746+
t.Run(name, func(t *testing.T) {
747+
cr := &v1alpha1.Subscription{
748+
Spec: v1alpha1.SubscriptionSpec{},
749+
}
750+
cr.Spec.ManagementPolicies = tc.managementPolicies
751+
got := isObserveOnly(cr)
752+
if got != tc.expected {
753+
t.Errorf("isObserveOnly() = %v, want %v", got, tc.expected)
754+
}
755+
})
756+
}
757+
}
758+
759+
func WithManagementPolicies(policies ...xpv1.ManagementAction) SubscriptionModifier {
760+
return func(r *v1alpha1.Subscription) {
761+
r.Spec.ManagementPolicies = policies
762+
}
763+
}

0 commit comments

Comments
 (0)