New Resource: aws_sagemaker_endpoint_configuration #2477

jckuester · 2017-11-30T00:08:28Z

Please only review the last commit. The reason is that this PR depends and is rebased on #2478, as the tests need the resource aws_sagemaker_endpoint_configuration to work.

Add the resource aws_sagemaker_endpoint_configuration for the newly announced service called SageMaker:

Documentation/Website
Schema
Implementation of CRUD operations
Acceptance tests

Acceptance tests

╰─$ make testacc TESTARGS='-run=TestAccAWSSagemakerEndpointConfig_*'                                                                     2 ↵
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSSagemakerEndpointConfig_* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSagemakerEndpointConfig_basic
--- PASS: TestAccAWSSagemakerEndpointConfig_basic (72.46s)
=== RUN   TestAccAWSSagemakerEndpointConfig_kmsKeyId
--- PASS: TestAccAWSSagemakerEndpointConfig_kmsKeyId (97.70s)
=== RUN   TestAccAWSSagemakerEndpointConfig_tags
--- PASS: TestAccAWSSagemakerEndpointConfig_tags (109.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	279.620s

Updates

14th Feb 2018: Support new parameter kms_key_id

mbfrahry

Hello again @jckuester! Things were fairly similar between this review and the model review but again things are looking good.

aws/resource_aws_sagemaker_endpoint_configuration.go

mbfrahry · 2018-11-13T00:41:31Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+			},
+
+			"production_variants": {
+				Type:     schema.TypeSet,


Is there a reason this is a TypeSet? If it's because this comes back from the api unordered then this is perfect to use. Otherwise we should use a TypeList as they are easier to access and use because of it's ordered nature.

mbfrahry · 2018-11-13T00:41:57Z

aws/resource_aws_sagemaker_endpoint_configuration.go

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


Is there a need for this attribute. I don't think it adds a ton of value inside of Terraform and can be removed.

mbfrahry · 2018-11-13T00:42:22Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+	}
+
+	d.SetId(name)
+	if err := d.Set("arn", resp.EndpointConfigArn); err != nil {


Since this is set in the read, I think we can remove it from here

mbfrahry · 2018-11-13T00:44:08Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+	result := make([]map[string]interface{}, 0, len(list))
+	for _, i := range list {
+		l := map[string]interface{}{
+			"variant_name":           *i.VariantName,


We should check that these aren't nil before setting them

mbfrahry · 2018-11-13T00:46:00Z

aws/resource_aws_sagemaker_endpoint_configuration_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckSagemakerEndpointConfigExists("aws_sagemaker_endpoint_configuration.foo",
+						&endpointConfig),
+					testAccCheckSagemakerEndpointConfigName(&endpointConfig,


I'm not sure we need these types of checks as it's the same as what you're checking below since the state is using the same object you're referencing here.

mbfrahry · 2018-11-13T00:46:32Z

website/docs/r/sagemaker_endpoint_configuration.html.markdown

+* `model_name` - (Required) The name of the model to use.
+* `initial_instance_count` - (Required) Initial number of instances used for auto-scaling.
+* `instance_type` (Required) - The type of instance to start.
+* `initial_variant_weight` - (Required)


We should add a brief description here

mbfrahry · 2018-11-13T00:47:58Z

website/docs/r/sagemaker_endpoint_configuration.html.markdown

+        variant_name            = "variant-1"
+        model_name              = "my-model"
+        initial_instance_count  = 1
+        instance_type           = ""


Can instance_type be ""? We should populate this with a valid value

jckuester · 2019-01-21T16:41:10Z

@mbfrahry I am done here, too, and ran tests.

Update:

@bflad I updated this PR based on the changes you made during merge on my PR #2478. Hope that helps.

xmj · 2019-03-18T11:50:12Z

How can we help to get this merged?

bflad

Hi @jckuester 👋 Thanks so much for this contribution. I'll be working through the rest of these resource reviews the coming week(s) as a high priority so we can finally get all these shipped. 🚢

A few more minor little things below, but otherwise looks pretty good! Please reach out with any questions or if you do not have time to implement the feedback here.

bflad · 2019-03-21T21:16:38Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+		}
+
+		if sagemakerErr.Code() == "ResourceNotFound" {
+			return resource.RetryableError(err)


Is there a particular reason to retry for 5 minutes for the the ResourceNotFound error? Seems like something we where we would want to exit successfully. 😄

If there's no reason to retry things here I would remove the resource.Retry() logic altogether and just simplify the deletion to:

func resourceAwsSagemakerEndpointConfigurationDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).sagemakerconn deleteOpts := &sagemaker.DeleteEndpointConfigInput{ EndpointConfigName: aws.String(d.Id()), } log.Printf("[INFO] Deleting Sagemaker endpoint configuration: %s", d.Id()) _, err := conn.DeleteEndpointConfig(deleteOpts) if isAWSErr(err, sagemaker.ErrCodeResourceNotFound, "") { return nil } if err != nil { return fmt.Errorf("error deleting SageMaker Endpoint Configuration (%s): %s", d.Id(), err) } return nil }

bflad · 2019-03-22T08:11:35Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+func resourceAwsSagemakerEndpointConfigurationUpdate(d *schema.ResourceData, meta interface{}) error {
+	conn := meta.(*AWSClient).sagemakerconn
+
+	d.Partial(true)


Nit: Since this resource does not require partial Terraform state updates, we should remove the usage of d.Partial() and d.SetPartial()

bflad · 2019-03-22T08:14:27Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+	})
+}
+
+func expandProductionVariants(configured []interface{}) ([]*sagemaker.ProductionVariant, error) {


Two things:

This function never returns an error so that should be removed from the function signature

Nit: Since this codebase uses a very flat Go package structure, we should include the AWS service name in the expand and flatten function names

Suggested change

func expandProductionVariants(configured []interface{}) ([]*sagemaker.ProductionVariant, error) {

func expandSagemakerProductionVariants(configured []interface{}) []*sagemaker.ProductionVariant {

bflad · 2019-03-22T08:15:20Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+		if v, ok := data["initial_variant_weight"]; ok {
+			l.InitialVariantWeight = aws.Float64(v.(float64))
+		} else {
+			l.InitialVariantWeight = aws.Float64(1)


Should we add this as a Default for the schema attribute?

bflad · 2019-03-22T08:18:11Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+
+	for _, i := range list {
+		l := map[string]interface{}{
+			"instance_type":          *i.InstanceType,


To prevent potential panics, we should prefer to use the AWS Go SDK provided dereferencing functions here and below. We can likely also remove nil checks below as well with them. e.g.

Suggested change

"instance_type": *i.InstanceType,

"instance_type": aws.StringValue(i.InstanceType),

bflad · 2019-03-22T08:23:58Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+				},
+			},
+
+			"kms_key_id": {


Since the Sagemaker API Reference notes this should be an Amazon Resource Name anyways, we should consider renaming this to kms_key_arn? We have been trying to start doing this places where we can to reduce confusion. We can also validate it partially with ValidateFunc: validateArn

bflad · 2019-03-22T08:29:33Z

aws/resource_aws_sagemaker_endpoint_configuration_test.go

+		}
+
+		resp, err := conn.ListEndpointConfigs(&sagemaker.ListEndpointConfigsInput{
+			NameContains: aws.String(rs.Primary.ID),


This seems like it can report false positives and should use DescribeEndpointConfig instead, e.g.

input := &sagemaker.DescribeEndpointConfigInput{ EndpointConfigName: aws.String(rs.Primary.ID), } output, err := conn.DescribeEndpointConfig(input) if isAWSErr(err, sagemaker.ErrCodeResourceNotFound, "") { continue } if err != nil { return err } return fmt.Errorf("Sagemaker Endpoint Configuration (%s) still exists", rs.Primary.ID)

bflad · 2019-03-22T08:32:03Z

aws/resource_aws_sagemaker_endpoint_configuration.go

+
+	endpointConfig, err := conn.DescribeEndpointConfig(request)
+	if err != nil {
+		if sagemakerErr, ok := err.(awserr.Error); ok && sagemakerErr.Code() == "ValidationException" {


Nit: We have a helper function for this type of checking:

Suggested change

if sagemakerErr, ok := err.(awserr.Error); ok && sagemakerErr.Code() == "ValidationException" {

if isAWSErr(err, "ValidationException", "") {

bflad · 2019-03-22T08:37:27Z

website/docs/r/sagemaker_endpoint_configuration.html.markdown

+
+* `name` - (Optional) The name of the endpoint configuration. If omitted, Terraform will assign a random, unique name.
+* `production_variants` - (Required) Fields are documented below.
+* `kms_key_id` - (Optional) KMS key to encrypt the model data.


Matching the Sagemaker API Reference (and suggested rename above)

Suggested change

* `kms_key_id` - (Optional) KMS key to encrypt the model data.

* `kms_key_arn` - (Optional) Amazon Resource Name (ARN) of a AWS Key Management Service key that Amazon SageMaker uses to encrypt data on the storage volume attached to the ML compute instance that hosts the endpoint.

bflad · 2019-03-22T08:44:50Z

website/docs/r/sagemaker_endpoint_configuration.html.markdown

+
+The `production_variant` block supports:
+
+* `variant_name` - (Optional) The name of the variant. If omitted, Terraform will assign a random, unique name.


Nit: These attributes should be alphabetically sorted either as a whole or required first then optional

jckuester · 2019-03-22T19:24:10Z

@bflad thanks for the feedback! I will implement your feedback within the next days (probably Monday).

jckuester · 2019-03-25T08:48:33Z

@bflad I am done here. See the last commit.

TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSSagemakerEndpointConfiguration* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
--- PASS: TestAccAWSSagemakerEndpointConfiguration_ProductionVariants_AcceleratorType (51.70s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_Basic (56.96s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_ProductionVariants_InitialVariantWeight (70.52s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_Tags (79.69s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_KmsKeyId (105.20s)

bflad

Looks great, thanks @jckuester! 🚀

--- PASS: TestAccAWSSagemakerEndpointConfiguration_Basic (35.87s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_Tags (46.12s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_ProductionVariants_AcceleratorType (50.91s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_ProductionVariants_InitialVariantWeight (62.43s)
--- PASS: TestAccAWSSagemakerEndpointConfiguration_KmsKeyId (66.03s)

bflad · 2019-03-29T06:35:18Z

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

ghost · 2020-03-30T17:58:25Z

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!

This was referenced Nov 30, 2017

New Resource: aws_sagemaker_model #2478

Merged

Add SageMaker support #2493

Closed

radeksimko added the new-resource Introduces a new resource. label Nov 30, 2017

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from a74dcc4 to e15e614 Compare November 30, 2017 22:24

Puneeth-n mentioned this pull request Dec 4, 2017

Bump aws-sdk-go to v1.12.39 #2527

Merged

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch 3 times, most recently from 3d5897f to 67d1a8f Compare December 7, 2017 23:35

jckuester mentioned this pull request Dec 12, 2017

Sagemaker example of "deploy your own model" #2585

Closed

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch 2 times, most recently from 1c4f357 to 81ec6a4 Compare December 14, 2017 14:31

jckuester changed the title ~~[WIP] Support new resource: sagemaker endpoint configuration~~ Support new resource: sagemaker endpoint configuration Dec 14, 2017

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 81ec6a4 to f74eae4 Compare December 18, 2017 11:30

jckuester mentioned this pull request Jan 7, 2018

New Resource: aws_sagemaker_endpoint #2479

Merged

radeksimko added the service/sagemaker Issues and PRs that pertain to the sagemaker service. label Jan 16, 2018

radeksimko changed the title ~~Support new resource: sagemaker endpoint configuration~~ New Resource: aws_sagemaker_endpoint_configuration Jan 16, 2018

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch 2 times, most recently from e7d5764 to df51483 Compare January 21, 2018 00:30

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from df51483 to 1043e89 Compare February 14, 2018 11:00

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

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 1043e89 to 04709d4 Compare February 14, 2018 11:11

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

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 04709d4 to a496c39 Compare February 14, 2018 11:22

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

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from a496c39 to 782eb14 Compare April 24, 2018 16:42

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

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 782eb14 to 6f318ea Compare April 26, 2018 19:10

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

mbfrahry suggested changes Nov 13, 2018

View reviewed changes

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 6f318ea to b4efa5a Compare January 9, 2019 21:42

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from ebfa00a to 5172ec3 Compare January 12, 2019 21:18

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 5172ec3 to 20cff99 Compare January 21, 2019 16:40

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 20cff99 to 694c1c2 Compare January 21, 2019 16:50

Add resource_aws_sagemaker_endpoint_configuration

0919b7d

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 694c1c2 to 3326c92 Compare February 7, 2019 20:55

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Feb 7, 2019

Address feedback

077f20d

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 3326c92 to 077f20d Compare February 7, 2019 21:55

bflad self-requested a review March 21, 2019 21:08

bflad self-assigned this Mar 21, 2019

bflad added this to the v2.4.0 milestone Mar 21, 2019

bflad reviewed Mar 22, 2019

View reviewed changes

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from 9e9b90b to b5f26a7 Compare March 24, 2019 17:00

Address 2nd round of feedback

6ecd49f

jckuester force-pushed the feature/resource-aws-sagemaker-endpoint-configuration branch from b5f26a7 to 6ecd49f Compare March 25, 2019 08:47

bflad approved these changes Mar 25, 2019

View reviewed changes

bflad merged commit 833e073 into hashicorp:master Mar 25, 2019

bflad added a commit that referenced this pull request Mar 25, 2019

Update CHANGELOG for #2477

435eaed

jckuester deleted the feature/resource-aws-sagemaker-endpoint-configuration branch March 25, 2019 13:54

jckuester mentioned this pull request Sep 2, 2019

Sagemaker example of "deploy your own model" #9948

Merged

ewbankkit mentioned this pull request Nov 7, 2019

Add 'FloatAtLeast' and 'FloatAtMost' validation functions hashicorp/terraform-plugin-sdk#239

Merged

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_sagemaker_endpoint_configuration #2477

New Resource: aws_sagemaker_endpoint_configuration #2477

jckuester commented Nov 30, 2017 •

edited

Loading

mbfrahry left a comment

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

mbfrahry Nov 13, 2018

jckuester Jan 9, 2019 •

edited

Loading

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

mbfrahry Nov 13, 2018

jckuester Dec 2, 2018

jckuester commented Jan 21, 2019 •

edited

Loading

xmj commented Mar 18, 2019

bflad left a comment

bflad Mar 21, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

bflad Mar 22, 2019

jckuester commented Mar 22, 2019 •

edited

Loading

jckuester commented Mar 25, 2019

bflad left a comment

bflad commented Mar 29, 2019

ghost commented Mar 30, 2020

	func expandProductionVariants(configured []interface{}) ([]*sagemaker.ProductionVariant, error) {
	func expandSagemakerProductionVariants(configured []interface{}) []*sagemaker.ProductionVariant {

	"instance_type": *i.InstanceType,
	"instance_type": aws.StringValue(i.InstanceType),

	if sagemakerErr, ok := err.(awserr.Error); ok && sagemakerErr.Code() == "ValidationException" {
	if isAWSErr(err, "ValidationException", "") {

	* `kms_key_id` - (Optional) KMS key to encrypt the model data.
	* `kms_key_arn` - (Optional) Amazon Resource Name (ARN) of a AWS Key Management Service key that Amazon SageMaker uses to encrypt data on the storage volume attached to the ML compute instance that hosts the endpoint.


		The `production_variant` block supports:

		* `variant_name` - (Optional) The name of the variant. If omitted, Terraform will assign a random, unique name.

New Resource: aws_sagemaker_endpoint_configuration #2477

New Resource: aws_sagemaker_endpoint_configuration #2477

Conversation

jckuester commented Nov 30, 2017 • edited Loading

Acceptance tests

Updates

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

jckuester Jan 9, 2019 • edited Loading

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

jckuester commented Jan 21, 2019 • edited Loading

xmj commented Mar 18, 2019

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

jckuester commented Mar 22, 2019 • edited Loading

jckuester commented Mar 25, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Mar 29, 2019

ghost commented Mar 30, 2020

jckuester commented Nov 30, 2017 •

edited

Loading

jckuester Jan 9, 2019 •

edited

Loading

jckuester commented Jan 21, 2019 •

edited

Loading

jckuester commented Mar 22, 2019 •

edited

Loading