New resource: aws_sagemaker_lifecycle_config #7585
Conversation
ghost
added
size/XL
There was a problem hiding this comment.
Hi @jckuester 👋 Thanks for this submission here as well and initial review provided below. Please reach out with any questions or if you do not have time to implement the feedback. 👍
| // (same for on_start) | ||
| if v, ok := d.GetOk("on_create"); ok { | ||
| hook := &sagemaker.NotebookInstanceLifecycleHook{Content: aws.String(v.(string))} | ||
| createOpts.SetOnCreate([]*sagemaker.NotebookInstanceLifecycleHook{hook}) |
There was a problem hiding this comment.
Nit: As a style preference across the codebase for consistency, we typically prefer the equals syntax over the Set functions 👍
| createOpts.SetOnCreate([]*sagemaker.NotebookInstanceLifecycleHook{hook}) | |
| createOpts.OnCreate = []*sagemaker.NotebookInstanceLifecycleHook{hook} |
|
|
||
| if v, ok := d.GetOk("on_start"); ok { | ||
| hook := &sagemaker.NotebookInstanceLifecycleHook{Content: aws.String(v.(string))} | ||
| createOpts.SetOnStart([]*sagemaker.NotebookInstanceLifecycleHook{hook}) |
There was a problem hiding this comment.
Nit: Same here: as a style preference across the codebase for consistency, we typically prefer the equals syntax over the Set functions 👍
| createOpts.SetOnStart([]*sagemaker.NotebookInstanceLifecycleHook{hook}) | |
| createOpts.OnStart = []*sagemaker.NotebookInstanceLifecycleHook{hook} |
| Computed: true, | ||
| }, | ||
|
|
||
| "name": { |
There was a problem hiding this comment.
Can you please add validation for this field? e.g.
ValidateFunc: validation.StringLenBetween(1, 63),There was a problem hiding this comment.
I'll use the func validateSagemakerName here to check also against the name pattern (https://github.com/awsdocs/amazon-sagemaker-developer-guide/blob/master/doc_source/API_CreateNotebookInstanceLifecycleConfig.md#request-parameters)
| "github.com/hashicorp/terraform/helper/validation" | ||
| ) | ||
|
|
||
| func resourceAwsSagemakerLifeCycleConfiguration() *schema.Resource { |
There was a problem hiding this comment.
Its unfortunately verbose, but we should probably follow the API naming here for the resource functions and resource name to clarify this is specifically for managing Notebook Instance Lifecycle Configurations. This is in case the SageMaker API introduces other "Lifecycle Configurations".
aws_sagemaker_notebook_instance_lifecycle_configuration
| func resourceAwsSagemakerLifeCycleConfiguration() *schema.Resource { | |
| func resourceAwsSagemakerNotebookInstanceLifeCycleConfiguration() *schema.Resource { |
| return fmt.Errorf("error setting name for SageMaker notebook instance lifecycle configuration (%s): %s", d.Id(), err) | ||
| } | ||
|
|
||
| if len(lifecycleConfig.OnCreate) > 0 { |
There was a problem hiding this comment.
To prevent potential panics, this should perform a nil check
| if len(lifecycleConfig.OnCreate) > 0 { | |
| if len(lifecycleConfig.OnCreate) > 0 && lifecycleConfig.OnCreate[0] != nil { |
| } | ||
| log.Printf("[INFO] Deleting SageMaker notebook instance lifecycle configuration: %s", d.Id()) | ||
|
|
||
| return resource.Retry(5*time.Minute, func() *resource.RetryError { |
There was a problem hiding this comment.
Since this resource.Retry() contains no return resource.RetryableError() conditions, the resource.Retry() logic should be removed.
| testAccCheckAWSSagemakerLifecycleConfigurationExists(resourceName, &lifecycleConfig), | ||
|
|
||
| resource.TestCheckResourceAttr(resourceName, "name", rName), | ||
| resource.TestCheckResourceAttr(resourceName, "on_create", base64Encode([]byte("echo foo"))), |
There was a problem hiding this comment.
The "basic" test configuration should omit any optional parameters and instead check for attribute zero-values or empty values, e.g.
resource.TestCheckNoResourceAttr(resourceName, "on_create"),
resource.TestCheckNoResourceAttr(resourceName, "on_start"),| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccSagemakerLifecycleConfigurationConfig_Basic(rName), | ||
| Check: resource.ComposeTestCheckFunc( |
There was a problem hiding this comment.
We should check the value of the arn attribute is populated correct in here, e.g.
testAccCheckResourceAttrRegionalARN(resourceName, "arn", "sagemaker", fmt.Sprintf("notebook-instance-lifecycle-config/%s", rName)),|
|
||
| if err != nil { | ||
| if isAWSErr(err, "ValidationException", "") { | ||
| return nil |
There was a problem hiding this comment.
CheckDestroy functions should use continue instead of return nil to allow checking multiple declarations of the resource.
| return fmt.Errorf("SageMaker notebook instance lifecycle configuration %s still exists", rs.Primary.ID) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Same here although the line can just be removed since it is the end of the for loop logic.
bflad
added
the
waiting-response
jckuester
force-pushed
the
feature/resource_aws_sagemaker_lifecycle_config
branch
from
9f35135
to
3e94716
Compare
ghost
added
size/XXL
jckuester
force-pushed
the
feature/resource_aws_sagemaker_lifecycle_config
branch
from
3e94716
to
fb7d50d
Compare
|
@bflad I addressed your feedback (see last commit). |
ghost
removed
the
waiting-response
There was a problem hiding this comment.
This looks great as well. Thanks @jckuester! 🚀
--- PASS: TestAccAWSSagemakerNotebookInstanceLifecycleConfiguration_Basic (10.36s)
--- PASS: TestAccAWSSagemakerNotebookInstanceLifecycleConfiguration_Update (15.74s)
|
This has been released in version 2.5.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
This PR adds the resource
aws_sagemaker_endpoint_configuration(including tests).