Sagemaker notebook instance #7139

mbfrahry · 2019-01-15T03:19:59Z

This PR starts the process to add the long requested sagemaker resources to the aws provider. Huge shoutout to @ddcprg in #2999 for getting the bulk of this done.

=== RUN   TestAccAWSSagemakerNotebookInstance_basic
=== PAUSE TestAccAWSSagemakerNotebookInstance_basic
=== RUN   TestAccAWSSagemakerNotebookInstance_update
=== PAUSE TestAccAWSSagemakerNotebookInstance_update
=== RUN   TestAccAWSSagemakerNotebookInstance_tags
=== PAUSE TestAccAWSSagemakerNotebookInstance_tags
=== CONT  TestAccAWSSagemakerNotebookInstance_basic
=== CONT  TestAccAWSSagemakerNotebookInstance_tags
=== CONT  TestAccAWSSagemakerNotebookInstance_update
--- PASS: TestAccAWSSagemakerNotebookInstance_tags (298.50s)
--- PASS: TestAccAWSSagemakerNotebookInstance_basic (431.74s)
--- PASS: TestAccAWSSagemakerNotebookInstance_update (483.99s)

…endor-notebook-instance

…aform-providers/terraform-provider-aws into sagemaker-notebook-instance

bflad

Hey @mbfrahry 👋 A few little things then this should be good to go!

bflad · 2019-01-15T11:50:10Z

aws/resource_aws_sagemaker_notebook_instance.go

+			"name": {
+				Type:         schema.TypeString,
+				Required:     true,
+				Computed:     false,


Nit: Computed: false is extraneous for attributes

bflad · 2019-01-15T11:51:43Z

aws/resource_aws_sagemaker_notebook_instance.go

+func resourceAwsSagemakerNotebookInstanceRead(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).sagemakerconn
+
+	// TODO change this to describeNotebook Instance


Old comment? If not, please add GitHub issue URL

bflad · 2019-01-15T11:53:58Z

aws/resource_aws_sagemaker_notebook_instance.go

+	}
+	notebookInstance, err := conn.DescribeNotebookInstance(describeNotebookInput)
+	if err != nil {
+		if isAWSErr(err, "", "RecordNotFound") {


Does SageMaker not return an ErrorCode? Seems odd compared to other AWS services and likely AWS Go SDK bug worthy. Might be best to create a _disappears acceptance test and verify this. 👍

All other sagemaker resources return ResourceNotFound except for notebook instance. It's definitely odd.

This is the error that gets returned when you try and do something with a notebook instance that has been deleted ValidationException: RecordNotFound

isAWSErr() does a strict check on the second parameter so isAWSErr(err, "ValidationException", "RecordNotFound") should do the trick then. 🎉

bflad · 2019-01-15T11:55:02Z

aws/resource_aws_sagemaker_notebook_instance.go

+	if err != nil {
+		if isAWSErr(err, "", "RecordNotFound") {
+			d.SetId("")
+			log.Printf("[LOG] Unable to find sageMaker notebook instance %q; removing from state file", d.Id())


Nit: [LOG] is not an hclog log level

Suggested change

log.Printf("[LOG] Unable to find sageMaker notebook instance %q; removing from state file", d.Id())

log.Printf("[WARN] Unable to find SageMaker Notebook Instance (%s), removing from state", d.Id())

bflad · 2019-01-15T11:55:49Z

aws/resource_aws_sagemaker_notebook_instance.go

+		ResourceArn: notebookInstance.NotebookInstanceArn,
+	})
+	if err != nil {
+		log.Printf("[ERR] error reading tags: %s", err)


Nit: We should return this in the error message instead of just logging it

bflad · 2019-01-15T12:02:52Z

aws/resource_aws_sagemaker_notebook_instance.go

+		return fmt.Errorf("unable to find sagemaker notebook instance to delete %q: %s", d.Id(), err)
+	}
+	if *notebook.NotebookInstanceStatus != sagemaker.NotebookInstanceStatusFailed {
+		if err := stopSagemakerNotebookInstance(conn, d.Id()); err != nil {


Should we check to see if its already stopped and skip this if it is? It looks like stopSagemakerNotebookInstance might already check this so the DescribeNotebookInstance call above may be extraneous.

Ah I guess stopSagemakerNotebookInstance returns different errors and whatnot. Adding the conditional for sagemaker.NotebookInstanceStatusStopped is probably best here. 👍

bflad · 2019-01-15T12:04:53Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+			continue
+		}
+
+		resp, err := conn.ListNotebookInstances(&sagemaker.ListNotebookInstancesInput{


Should this be using DescribeNotebookInstance to match the Read function and Exists test function? A name contains check seems more flakey than directly checking.

bflad · 2019-01-15T12:09:47Z

website/docs/r/sagemaker_notebook_instance.html.markdown

+
+## Import
+
+Models can be imported using the `name`, e.g.


🍝

Suggested change

Models can be imported using the `name`, e.g.

SageMaker Notebook Instances can be imported using the `name`, e.g.

bflad · 2019-01-15T12:10:45Z

aws/resource_aws_sagemaker_notebook_instance.go

+		}
+		notebook, err := conn.DescribeNotebookInstance(describeNotebookInput)
+		if err != nil {
+			if sagemakerErr, ok := err.(awserr.Error); ok && sagemakerErr.Message() == "RecordNotFound" {


Should this be RecordNotFound or ResourceNotFound to match others?

RecordNotFound is the correct usage here

bflad · 2019-01-15T12:12:31Z

aws/resource_aws_sagemaker_notebook_instance.go

+
+	}
+
+	d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups))


Should add error checking here to match everything else 👍

Suggested change

d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups))

if err := d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups)); err != nil {

return fmt.Errorf("error setting security_groups for sagemaker notebook instance %q: %s", d.Id(), err)

}

bflad

🎉 🎉 🎉 Thanks, @ddcprg and @mbfrahry!

--- PASS: TestAccAWSSagemakerNotebookInstance_disappears (236.38s)
--- PASS: TestAccAWSSagemakerNotebookInstance_basic (236.85s)
--- PASS: TestAccAWSSagemakerNotebookInstance_tags (403.87s)
--- PASS: TestAccAWSSagemakerNotebookInstance_update (570.45s)

bflad · 2019-01-16T04:16:13Z

This has been released in version 1.56.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ddcprg · 2019-01-18T10:31:57Z

Awesome @mbfrahry and @bflad !!! Massive thanks for taking over 🤘

ddcprg · 2019-01-22T17:59:57Z

Hi @mbfrahry and @bflad the aws_sagemaker_training_job resource was in this branch but was not merged together with the aws_sagemaker_notebook_instance resource. The PR #2955 also contains the resource implementation. Are you planning to include this soon?

mbfrahry · 2019-01-23T20:01:08Z

Hey @ddcprg, apologies for not surfacing this earlier. I have the training job piece cleaned up in a branch but I'm not sure it's the best fit for a resource in Terraform. Resources in Terraform are thought of as immutable infrastructure to manage some application . Training jobs go against that by acting like an application. We fire it off, let it run to completion, and then go over the results of what happened. I believe another tool would be better suited for training jobs but I'm open to adding this if you or others feel strongly about it living inside of Terraform.

ddcprg · 2019-01-24T09:56:54Z

Thanks for the insight @mbfrahry that makes sense and if that's the philosophy of the project then I'm fine with keeping it out or leave the decision to the community. Cheers!

ghost · 2020-04-01T17:23:08Z

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!

Daniel del Castillo and others added 18 commits January 30, 2018 10:52

[issue-2493] SageMaker Training Job resource

69f3089

[issue-2493] SageMaker Training Job resource

4f9e248

Merge branch 'master' into feature/sagemaker-vendor-notebook-instance

a865552

Merge branch 'master' into feature/sagemaker-vendor-notebook-instance

069d5b7

Merge remote-tracking branch 'origin/master' into feature/sagemaker-v…

8912065

…endor-notebook-instance

[issue-2493] Sidebar section in docs

51774eb

Merge branch 'master' into feature/sagemaker-vendor-notebook-instance

e2b8225

Fixed merge conflict

5eabd23

working on review

3d80117

more working on review

b8944fa

Merge branch 'sagemaker-notebook-instance' of https://github.com/terr…

64a6842

…aform-providers/terraform-provider-aws into sagemaker-notebook-instance

Adding stateChangeConf support

590dbfe

Mostly finished review

de3262c

Fixing delete

16fd2bf

Finishing notebook instance refresh

ed7d0bb

Merge branch 'sagemaker-notebook-instance' of https://github.com/terr…

2c83eb6

…aform-providers/terraform-provider-aws into sagemaker-notebook-instance

final changes

f3a81fd

Removing training job

11c5900

Merge branch 'master' into sagemaker-notebook-instance

b77b62a

mbfrahry requested a review from a team January 15, 2019 03:21

mbfrahry added 3 commits January 14, 2019 21:14

Fixing linting issue

6edfbbd

Merge branch 'sagemaker-notebook-instance' of https://github.com/terr…

4141e6f

…aform-providers/terraform-provider-aws into sagemaker-notebook-instance

Fixing website link

ec585ed

bflad reviewed Jan 15, 2019

View reviewed changes

bflad added the new-resource Introduces a new resource. label Jan 15, 2019

Addressing comments

44642f7

bflad approved these changes Jan 16, 2019

View reviewed changes

bflad added this to the v1.56.0 milestone Jan 16, 2019

bflad merged commit af1244b into master Jan 16, 2019

bflad added a commit that referenced this pull request Jan 16, 2019

Update CHANGELOG for #7139

46c722b

mbfrahry deleted the sagemaker-notebook-instance branch January 17, 2019 03:56

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sagemaker notebook instance #7139

Sagemaker notebook instance #7139

mbfrahry commented Jan 15, 2019 •

edited

Loading

bflad left a comment

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

mbfrahry Jan 15, 2019

mbfrahry Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

bflad Jan 15, 2019

mbfrahry Jan 15, 2019

bflad Jan 15, 2019

bflad left a comment

bflad commented Jan 16, 2019

ddcprg commented Jan 18, 2019

ddcprg commented Jan 22, 2019

mbfrahry commented Jan 23, 2019

ddcprg commented Jan 24, 2019

ghost commented Apr 1, 2020

	log.Printf("[LOG] Unable to find sageMaker notebook instance %q; removing from state file", d.Id())
	log.Printf("[WARN] Unable to find SageMaker Notebook Instance (%s), removing from state", d.Id())

	Models can be imported using the `name`, e.g.
	SageMaker Notebook Instances can be imported using the `name`, e.g.


		}

		d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups))

-	d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups))
+	if err := d.Set("security_groups", flattenStringList(notebookInstance.SecurityGroups)); err != nil {
+		return fmt.Errorf("error setting security_groups for sagemaker notebook instance %q: %s", d.Id(), err)
+	}

Sagemaker notebook instance #7139

Sagemaker notebook instance #7139

Conversation

mbfrahry commented Jan 15, 2019 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jan 16, 2019

ddcprg commented Jan 18, 2019

ddcprg commented Jan 22, 2019

mbfrahry commented Jan 23, 2019

ddcprg commented Jan 24, 2019

ghost commented Apr 1, 2020

mbfrahry commented Jan 15, 2019 •

edited

Loading