New Resource: aws_sagemaker_notebook_instance #2999

ddcprg · 2018-01-15T18:02:18Z

Helps resolving #2493

Shugyousha · 2018-01-26T11:59:46Z

aws/resource_aws_sagemaker_notebook_instance.go

+	} else {
+		name = v.(string)
+	}
+


This else clause is not needed since we return in the error case anyway.

Fixed, I'll fix the training job as well

ramin · 2018-01-26T18:37:40Z

aws/resource_aws_sagemaker_notebook_instance.go

+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/helper/schema"
+	"log"
+	"time"


The common pattern in this codebase is to separate stdlib imports from others, so these across this PR should be

"fmt" "log" "time" "github.com/aws/aws-sdk-go/aws" ...

etc, eg: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_api_gateway_api_key_test.go#L4

FYI goimports is the tool that does this automatically (personally I have it setup in my editor on save): https://godoc.org/golang.org/x/tools/cmd/goimports

Thanks both, I'll fix this and the other PR today

toyama0919 · 2018-03-15T09:20:40Z

How is this PR going?

ddcprg · 2018-03-22T15:49:44Z

The build has failed due to S3 errors in aws/validators_test.go

ddcprg · 2018-05-16T09:47:19Z

The build error is unrelated to this PR

bcornils · 2018-06-11T22:47:39Z

Looks like we have a check failing. Were we able to identify the criticality of it?

ddcprg · 2018-06-12T16:52:46Z

@bcornils I'll merge master and build again, the error was unrelated to this change last time I checked

…endor-notebook-instance

ddcprg · 2018-06-13T08:56:59Z

I was missing the sidebar in the docs so I've added it. I've merged master into this branch and the build still fails with this error:

# github.com/terraform-providers/terraform-provider-aws/aws
aws/validators_test.go:559:16: undefined: validateS3BucketLifecycleExpirationDays
aws/validators_test.go:571:16: undefined: validateS3BucketLifecycleExpirationDays
aws/validators_test.go:587:16: undefined: validateS3BucketLifecycleTransitionDays
aws/validators_test.go:598:16: undefined: validateS3BucketLifecycleTransitionDays
aws/validators_test.go:612:13: cannot assign 1 values to 2 variables
aws/validators_test.go:612:53: too many arguments in call to validateS3BucketLifecycleStorageClass
	have (string, string)
	want ()
aws/validators_test.go:623:13: cannot assign 1 values to 2 variables
aws/validators_test.go:623:53: too many arguments in call to validateS3BucketLifecycleStorageClass
	have (string, string)
	want ()
aws/validators_test.go:639:16: undefined: validateS3BucketReplicationRuleId
aws/validators_test.go:650:16: undefined: validateS3BucketReplicationRuleId
aws/validators_test.go:650:16: too many errors
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
FAIL	github.com/terraform-providers/terraform-provider-aws/aws [build failed]
make: *** [test] Error 2

Same issue in PR #2955

ddcprg · 2018-06-13T09:16:45Z

@bflad can we get some eyes on this PR?

ibash · 2018-07-17T22:05:30Z

🙌

nhjiejan · 2018-09-21T13:18:07Z

Any Update on this? working version available yet?

ddcprg · 2018-09-21T13:50:28Z

we've not received any feedback from Hashicorp

rainmanh · 2018-10-05T10:12:52Z

Updates? When will be sagemaker ready?

mbfrahry

Hello @ddcprg, I'm sorry for the delay on reviewing this PR. This review is fairly large but the design looks great. Feel free to leave any comments on concerns on any of the points mentioned and we'll get this merged in soon

mbfrahry · 2018-11-02T15:03:26Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+}
+
+resource "aws_iam_role" "foo" {
+	name = "terraform-testacc-sagemaker-foo"


If this test fails, the rest of the tests will fail as well since this name isn't randomized. Do you mind randomizing this and all other names to prevent one test failure bringing the rest of the tests down?

mbfrahry · 2018-11-02T15:24:05Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+}
+
+resource "aws_iam_role" "foo" {
+	name = "terraform-testacc-sagemaker-foo"


Same randomization note as above

mbfrahry · 2018-11-02T15:24:15Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+}
+
+resource "aws_iam_role" "foo" {
+	name = "terraform-testacc-sagemaker-foo"


Same randomization note as above.

mbfrahry · 2018-11-02T15:24:32Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+}
+
+resource "aws_iam_role" "foo" {
+	name = "terraform-testacc-sagemaker-foo"


Same randomization note as above

mbfrahry · 2018-11-02T15:27:14Z

aws/resource_aws_sagemaker_notebook_instance.go

+				ForceNew: true,
+			},
+
+			"creation_time": {


I don't believe time attributes carry the most beneficial of information that Terraform needs to keep track of it. Is there a good reason for having it in this resource or can we remove it?

mbfrahry · 2018-11-08T15:24:24Z

aws/resource_aws_sagemaker_training_job.go

+	}
+	log.Printf("[INFO] Stopping Sagemaker training job: %s", d.Id())
+
+	return resource.Retry(5*time.Minute, func() *resource.RetryError {


I'd suggest using the StateChangeConf method noted above.

mbfrahry · 2018-11-08T15:25:18Z

aws/resource_aws_sagemaker_training_job_test.go

+
+const sagemakerTestAccSagemakerTrainingJobResourceNamePrefix = "terraform-testacc-"
+
+func init() {


It looks like this sweeper is unfinished and unneeded since we can't delete the training job. Do you mind removing this?

mbfrahry · 2018-11-08T15:25:57Z

aws/resource_aws_sagemaker_training_job_test.go

+	trainingJobName := resource.PrefixedUniqueId(sagemakerTestAccSagemakerTrainingJobResourceNamePrefix)
+	bucketName := resource.PrefixedUniqueId(sagemakerTestAccSagemakerTrainingJobResourceNamePrefix)
+
+	resource.Test(t, resource.TestCase{


We can set all of these resource.Test to resource.ParrallelTest

mbfrahry · 2018-11-08T15:33:09Z

aws/resource_aws_sagemaker_training_job.go

+		log.Printf("[ERR] Error setting Stopping Condition: %s", err)
+		return err
+	}
+	if err := d.Set("hyper_parameters", fromStringPMapToTerraformMapSagemaker(&trainingJob.HyperParameters)); err != nil {


We can use the helper for maps instead of this custom function.

mbfrahry · 2018-11-08T15:36:28Z

aws/resource_aws_sagemaker_training_job.go

+		return err
+	}
+
+	d.Set("tags", tagsToMapSagemaker(tagsOutput.Tags))


Just in case, we should check for an error while setting tags

mbfrahry · 2019-01-10T14:43:32Z

Hey all. These changes from this review are being tracked in a new branch here

ghost · 2020-04-01T17:29:09Z

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!

ddcprg mentioned this pull request Jan 15, 2018

Add SageMaker support #2493

Closed

radeksimko added service/sagemaker Issues and PRs that pertain to the sagemaker service. new-resource Introduces a new resource. labels Jan 16, 2018

radeksimko changed the title ~~New resource: SageMaker Notebook Instance~~ New Resource: aws_sagemaker_notebook_instance Jan 16, 2018

Shugyousha reviewed Jan 26, 2018

View reviewed changes

ddcprg force-pushed the feature/sagemaker-vendor-notebook-instance branch 2 times, most recently from aa694da to c0d40d6 Compare January 26, 2018 14:59

ramin reviewed Jan 26, 2018

View reviewed changes

Daniel del Castillo added 2 commits January 30, 2018 10:52

[issue-2493] SageMaker Training Job resource

69f3089

[issue-2493] SageMaker Training Job resource

4f9e248

ddcprg force-pushed the feature/sagemaker-vendor-notebook-instance branch from c0d40d6 to 4f9e248 Compare January 30, 2018 10:54

ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jan 30, 2018

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

ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jun 12, 2018

[issue-2493] Sidebar section in docs

51774eb

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

e2b8225

ghost added the documentation Introduces or discusses updates to documentation. label Nov 1, 2018

ghost added provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 1, 2018

mbfrahry suggested changes Nov 8, 2018

View reviewed changes

mbfrahry self-assigned this Jan 10, 2019

mbfrahry mentioned this pull request Jan 15, 2019

Sagemaker notebook instance #7139

Merged

bflad merged commit e2b8225 into hashicorp:master Jan 16, 2019

StuartLox mentioned this pull request Feb 16, 2019

Enhancement/add lifecycle notebook sm #7586

Merged

bcatubig mentioned this pull request Mar 10, 2019

[WIP] Sagemaker Notebook DirectInternetAccess support #7884

Closed

WriteCodeEveryday mentioned this pull request Sep 23, 2019

Enhancement: Elastic Inference Accelerators for SageMaker #10208

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_sagemaker_notebook_instance #2999

New Resource: aws_sagemaker_notebook_instance #2999

ddcprg commented Jan 15, 2018

Shugyousha Jan 26, 2018

ddcprg Jan 26, 2018

ramin Jan 26, 2018

bflad Jan 26, 2018

ddcprg Jan 30, 2018 •

edited

Loading

toyama0919 commented Mar 15, 2018

ddcprg commented Mar 22, 2018

ddcprg commented May 16, 2018

bcornils commented Jun 11, 2018

ddcprg commented Jun 12, 2018

ddcprg commented Jun 13, 2018

ddcprg commented Jun 13, 2018 •

edited

Loading

ibash commented Jul 17, 2018

nhjiejan commented Sep 21, 2018

ddcprg commented Sep 21, 2018

rainmanh commented Oct 5, 2018

mbfrahry left a comment

mbfrahry Nov 2, 2018

mbfrahry Nov 2, 2018

mbfrahry Nov 2, 2018

mbfrahry Nov 2, 2018

mbfrahry Nov 2, 2018

mbfrahry Nov 8, 2018

mbfrahry Nov 8, 2018

mbfrahry Nov 8, 2018

mbfrahry Nov 8, 2018

mbfrahry Nov 8, 2018

mbfrahry commented Jan 10, 2019

ghost commented Apr 1, 2020


		const sagemakerTestAccSagemakerTrainingJobResourceNamePrefix = "terraform-testacc-"

		func init() {

New Resource: aws_sagemaker_notebook_instance #2999

New Resource: aws_sagemaker_notebook_instance #2999

Conversation

ddcprg commented Jan 15, 2018

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

ddcprg Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

toyama0919 commented Mar 15, 2018

ddcprg commented Mar 22, 2018

ddcprg commented May 16, 2018

bcornils commented Jun 11, 2018

ddcprg commented Jun 12, 2018

ddcprg commented Jun 13, 2018

ddcprg commented Jun 13, 2018 • edited Loading

ibash commented Jul 17, 2018

nhjiejan commented Sep 21, 2018

ddcprg commented Sep 21, 2018

rainmanh commented Oct 5, 2018

mbfrahry 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

mbfrahry commented Jan 10, 2019

ghost commented Apr 1, 2020

ddcprg Jan 30, 2018 •

edited

Loading

ddcprg commented Jun 13, 2018 •

edited

Loading