ADdress comments

Author: michaelhtm

pkg/runtime/reconciler.go

@@ -303,18 +303,21 @@ func (r *resourceReconciler) handleCacheError(
 	return r.HandleReconcileError(ctx, desired, latest, requeue.NeededAfter(err, roleARNNotAvailableRequeueDelay))
 }
 
+// Handles the population of ACK Resource fields from annotation.
 func (r *resourceReconciler) handlePopulation(
 	rlog acktypes.Logger,
 	desired acktypes.AWSResource,
 	adoptionPolicy AdoptionPolicy,
 ) (acktypes.AWSResource, error) {
 	// If the resource is being adopted by force, we need to access
 	// the required field passed by annotation and attempt a read.
-
-	rlog.Info("Adopting Resource")
+	rlog.Debug("Populating Resource")
 	adoptionFields, err := ExtractAdoptionFields(desired)
 	if err != nil {
-		if adoptionPolicy == AdoptOrCreatePolicy {
+		// if the user does not provide adoption fields and the policy
+		// is adopt-or-create, we will assume the adoption fields are passed
+		// in Spec
+		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
 			return desired, nil
 		}
 		return desired, ackerr.NewTerminalError(err)
@@ -323,9 +326,10 @@ func (r *resourceReconciler) handlePopulation(
 	populated := desired.DeepCopy()
 	err = populated.PopulateResourceFromAnnotation(adoptionFields)
 	// maybe don't return errors when it's adopt-or-create?
+	//
 	// TODO (michaelhtm) change PopulateResourceFromAnnotation to understand
 	// adopt-or-create, and validate Spec fields are not nil...
-	if err != nil && adoptionPolicy != AdoptOrCreatePolicy {
+	if err != nil && adoptionPolicy != AdoptionPolicy_AdoptOrCreate {
 		return nil, err
 	}
 
@@ -398,7 +402,8 @@ func (r *resourceReconciler) Sync(
 	isAdopted := IsAdopted(desired)
 	rlog.WithValues("is_adopted", isAdopted)
 
-	// find out if we need to adopt early on
+	// If AdoptionAnnotation feature gate is enabled, and the resource has an adoption
+	// annotation and doesn't hold an ACK finalizer, trigger the adoption path.
 	needAdoption := NeedAdoption(desired) && !r.rd.IsManaged(desired) && r.cfg.FeatureGates.IsEnabled(featuregate.ResourceAdoption)
 	adoptionPolicy, err := GetAdoptionPolicy(desired)
 	if err != nil {
@@ -430,7 +435,7 @@ func (r *resourceReconciler) Sync(
 		if err != nil {
 			return nil, err
 		}
-		if adoptionPolicy == AdoptOrCreatePolicy {
+		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
 			// here we assume the spec fields are provided in the spec.
 			resolved.SetStatus(populated)
 		} else {
@@ -454,7 +459,7 @@ func (r *resourceReconciler) Sync(
 		if err != ackerr.NotFound {
 			return latest, err
 		}
-		if adoptionPolicy == AdoptPolicy || isAdopted {
+		if adoptionPolicy == AdoptionPolicy_Adopt || isAdopted {
 			return nil, ackerr.AdoptedResourceNotFound
 		}
 		if isReadOnly {
@@ -463,7 +468,7 @@ func (r *resourceReconciler) Sync(
 		if latest, err = r.createResource(ctx, rm, resolved); err != nil {
 			return latest, err
 		}
-	} else if adoptionPolicy == AdoptPolicy {
+	} else if adoptionPolicy == AdoptionPolicy_Adopt {
 		rm.FilterSystemTags(latest)
 		if err = r.setResourceManaged(ctx, rm, latest); err != nil {
 			return latest, err
@@ -473,7 +478,9 @@ func (r *resourceReconciler) Sync(
 			return latest, err
 		}
 	} else if !isReadOnly {
-		if adoptionPolicy == AdoptOrCreatePolicy {
+		if adoptionPolicy == AdoptionPolicy_AdoptOrCreate {
+			// set adopt-or-create resource as managed before attempting
+			// update
 			if err = r.setResourceManaged(ctx, rm, latest); err != nil {
 				return latest, err
 			}

pkg/runtime/reconciler_test.go

@@ -324,7 +324,6 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) {
 	require := require.New(t)
 
 	ctx := context.TODO()
-	// arn := ackv1alpha1.AWSResourceName("my-adopt-book-arn")
 
 	desired, _, metaObj := resourceMocks()
 	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
@@ -334,7 +333,6 @@ func TestReconcilerAdoptOrCreateResource_Create(t *testing.T) {
 	})
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
-	// ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, _ := resourceMocks()
 	latest.On("Identifiers").Return(ids)
@@ -394,7 +392,6 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 	require := require.New(t)
 
 	ctx := context.TODO()
-	// arn := ackv1alpha1.AWSResourceName("my-adopt-book-arn")
 
 	desired, _, metaObj := resourceMocks()
 	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
@@ -404,7 +401,6 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) {
 	})
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
-	// ids.On("ARN").Return(&arn)
 	delta := ackcompare.NewDelta()
 	delta.Add("Spec.A", "val1", "val2")
 

pkg/runtime/util.go

@@ -29,15 +29,16 @@ import (
 
 // AdoptionPolicy stores adoptionPolicy values we expect users to
 // provide in the resources `adoption-policy` annotation
+//
 // TODO(michaelhtm) Maybe we need a different place for this...
 // next refactor maybe? 🤷‍♂️
 type AdoptionPolicy string
 
 const (
-	AdoptPolicy AdoptionPolicy = "adopt"
-	// calling this policy constant `AdoptOrCreatePolicy`
-	//  and see if we need to rename it!!!
-	AdoptOrCreatePolicy AdoptionPolicy = "adopt-or-create"
+	// AdoptPolicy is ...
+	AdoptionPolicy_Adopt AdoptionPolicy = "adopt"
+	// AdoptPolicy is ...
+	AdoptionPolicy_AdoptOrCreate AdoptionPolicy = "adopt-or-create"
 )
 
 // IsAdopted returns true if the supplied AWSResource was created with a
@@ -100,7 +101,7 @@ func GetAdoptionPolicy(res acktypes.AWSResource) (AdoptionPolicy, error) {
 		return "", nil
 	}
 
-	if policy != string(AdoptPolicy) && policy != string(AdoptOrCreatePolicy) {
+	if policy != string(AdoptionPolicy_Adopt) && policy != string(AdoptionPolicy_AdoptOrCreate) {
 		return "", fmt.Errorf("unrecognized adoption policy")
 	}