Notebook instance - Handling update only parameters (#82)

This pull request should be merged after #61, it contains code for handling cases where (lifecycleConfigName, AcceleratorTypes, DefaultCodeRepository, AdditionalCodeRepositories) are nil in desired and not nil in latest. In other words it allows the user to delete (lifecycleConfigName, AcceleratorTypes, DefaultCodeRepository, AdditionalCodeRepositories).

TODO: In test/e2e/tests/test_notebook_instance.py, we should use replace_custom_resource when test-infra supports it.

Files changed from #61 :
**generator.yaml:** Added a sdk_update_post_build_request hook that calls the handleUpdateOnlyParameters function
**pkg/resource/notebook_instance/sdk.go:** Added code that calls handleUpdateOnlyParameters
**test/e2e/tests/test_notebook_instance.py:** Added a test for Update only parameters.
**test/e2e/resources/notebook_instance.yaml:** Added the a field to test the update only parameters.

**pkg/resource/notebook_instance/custom_set_input_update.go:** Contains the function handleUpdateOnlyParameters which checks if the (lifecycleConfigName, AcceleratorTypes, DefaultCodeRepository, AdditionalCodeRepositories) are nil, if they are nil in the desired resource and not nil in the latest resource it sets the respective Disassociate<field> to true for the update input.

Author: ananth102

apis/v1alpha1/generator.yaml

@@ -658,6 +658,8 @@ resources:
         code: customSetDefaults(a, b)
       sdk_update_pre_build_request:
         template_path: notebook_instance/sdk_update_pre_build_request.go.tpl
+      sdk_update_post_build_request:
+        code: handleUpdateOnlyParameters(desired, latest, input)
       sdk_update_post_set_output:
         code: rm.customSetOutputUpdate(ko, latest)
       sdk_delete_pre_build_request:

generator.yaml

@@ -658,6 +658,8 @@ resources:
         code: customSetDefaults(a, b)
       sdk_update_pre_build_request:
         template_path: notebook_instance/sdk_update_pre_build_request.go.tpl
+      sdk_update_post_build_request:
+        code: handleUpdateOnlyParameters(desired, latest, input)
       sdk_update_post_set_output:
         code: rm.customSetOutputUpdate(ko, latest)
       sdk_delete_pre_build_request:

pkg/resource/notebook_instance/custom_set_input_update.go

@@ -0,0 +1,28 @@
+package notebook_instance
+
+import (
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	"github.com/aws/aws-sdk-go/aws"
+	svcsdk "github.com/aws/aws-sdk-go/service/sagemaker"
+)
+
+// handleUpdateOnlyParameters sets Disassociate<field> to true if corresponding value in desired is nil(ie not included in the spec)
+// and if the corresponding value in the latest spec is not nil (ie contains some value)
+// Ex if LifecycleConfigName was "a" in the latest spec but nil in the desired spec, DisassociateLifecycleConfig is set to true
+func handleUpdateOnlyParameters(
+	desired *resource,
+	latest *resource,
+	update_input *svcsdk.UpdateNotebookInstanceInput) {
+	if ackcompare.IsNil(desired.ko.Spec.AcceleratorTypes) && ackcompare.IsNotNil(latest.ko.Spec.AcceleratorTypes) {
+		update_input.DisassociateAcceleratorTypes = aws.Bool(true)
+	}
+	if ackcompare.IsNil(desired.ko.Spec.AdditionalCodeRepositories) && ackcompare.IsNotNil(latest.ko.Spec.AdditionalCodeRepositories) {
+		update_input.DisassociateAdditionalCodeRepositories = aws.Bool(true)
+	}
+	if ackcompare.IsNil(desired.ko.Spec.DefaultCodeRepository) && ackcompare.IsNotNil(latest.ko.Spec.DefaultCodeRepository) {
+		update_input.DisassociateDefaultCodeRepository = aws.Bool(true)
+	}
+	if ackcompare.IsNil(desired.ko.Spec.LifecycleConfigName) && ackcompare.IsNotNil(latest.ko.Spec.LifecycleConfigName) {
+		update_input.DisassociateLifecycleConfig = aws.Bool(true)
+	}
+}

pkg/resource/notebook_instance/sdk.go

@@ -1,10 +1,15 @@
-// NotebookInstanceLifecycleConfigName does not get generated by the code generator.
-// because the field name in spec vs Describe response do not match
+// NotebookInstanceLifecycleConfigName and SecurityGroups do not get generated by the code generator.
+// Because the field name in spec vs Describe response do not match
 if resp.NotebookInstanceLifecycleConfigName != nil {
 	ko.Spec.LifecycleConfigName = resp.NotebookInstanceLifecycleConfigName
 } else {
 	ko.Spec.LifecycleConfigName = nil
 }
+if resp.SecurityGroups != nil {
+	ko.Spec.SecurityGroupIDs = resp.SecurityGroups
+} else {
+	ko.Spec.SecurityGroupIDs = nil
+}
 err = rm.customSetOutputDescribe(&resource{ko})
 if err != nil{
   return nil, err

templates/notebook_instance/sdk_read_one_post_set_output.go.tpl

@@ -8,4 +8,5 @@ spec:
   roleARN: $SAGEMAKER_EXECUTION_ROLE_ARN
   volumeSizeInGB: 6
   directInternetAccess: Enabled
-  rootAccess: Enabled
+  rootAccess: Enabled
+  defaultCodeRepository: $DEFAULT_CODE_REPOSITORY

test/e2e/resources/notebook_instance.yaml

@@ -35,9 +35,11 @@
 
 @pytest.fixture(scope="module")
 def notebook_instance():
+    default_code_repository = "https://github.com/aws-controllers-k8s/community"
     resource_name = random_suffix_name("nb", 32)
     replacements = REPLACEMENT_VALUES.copy()
     replacements["NOTEBOOK_INSTANCE_NAME"] = resource_name
+    replacements["DEFAULT_CODE_REPOSITORY"] = default_code_repository
     reference, spec, resource = create_sagemaker_resource(
         resource_plural="notebookinstances",
         resource_name=resource_name,
@@ -160,8 +162,13 @@ def update_notebook_test(self, notebook_instance):
         (reference, resource, spec) = notebook_instance
         notebook_instance_name = resource["spec"].get("notebookInstanceName", None)
         volumeSizeInGB = 7
+        additionalCodeRepositories = ["https://github.com/aws-controllers-k8s/runtime"]
 
         spec["spec"]["volumeSizeInGB"] = volumeSizeInGB
+        # TODO: Use del spec["spec"]["defaultCodeRepository"] instead once ack.testinfra supports replacement.
+        # Patch only supports updating spec fields instead of fully getting rid of them.
+        spec["spec"]["defaultCodeRepository"] = None
+        spec["spec"]["additionalCodeRepositories"] = additionalCodeRepositories
         k8s.patch_custom_resource(reference, spec)
 
         self._assert_notebook_status_in_sync(
@@ -183,6 +190,12 @@ def update_notebook_test(self, notebook_instance):
         resource = k8s.get_resource(reference)
         assert resource["spec"]["volumeSizeInGB"] == volumeSizeInGB
 
+        assert "DefaultCodeRepository" not in notebook_instance_desc
+        assert "defaultCodeRepository" not in resource["spec"]
+
+        assert resource["spec"]["additionalCodeRepositories"] == additionalCodeRepositories
+        assert notebook_instance_desc["AdditionalCodeRepositories"] == additionalCodeRepositories
+
         assert "stoppedByControllerMetadata" not in resource["status"]
 
     def delete_notebook_test(self, notebook_instance):