-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Resource: aws_sagemaker_model #2478
New Resource: aws_sagemaker_model #2478
Conversation
1d5347f
to
18a6eaa
Compare
4fe7010
to
e41d067
Compare
24b6718
to
48fd36a
Compare
17e732d
to
f7be35d
Compare
f7be35d
to
f4d0931
Compare
4386895
to
8ba124e
Compare
0907783
to
7468b70
Compare
@radeksimko waiting for feedback on this PR :-) Thanks. |
6c17182
to
643b09b
Compare
643b09b
to
4940787
Compare
4940787
to
1a815f3
Compare
650aba6
to
52d9a31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a final pass against the latest code commits. It looks like a needed function was removed
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckSagemakerModelExists("aws_sagemaker_model.foo"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jckuester, was there any reason why this and the corresponding method was removed? It's a fairly important check for the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readded.
e9626f3
to
b5a5549
Compare
6ad9a40
to
4049269
Compare
@mbfrahry I guess, I addressed all your comments and ran the tests (which are green now). I hope we are ready to go :) Somehow the linter fails (https://travis-ci.org/terraform-providers/terraform-provider-aws/builds/482829006#L512) with |
4049269
to
d64ad64
Compare
Hey @jckuester, apologies for making some minor changes to the PR. The changes you've made look great and I just made one more comment if you don't mind looking at it and then we can merge it in |
de5f4b9
to
73037d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling this in so we can get this released today and will make the very minor changes below on merge. Thanks so much for sticking through this one @jckuester and @mbfrahry! 🚀
Output from acceptance testing:
--- PASS: TestAccAWSSagemakerModel_primaryContainerEnvironment (23.40s)
--- PASS: TestAccAWSSagemakerModel_basic (23.70s)
--- PASS: TestAccAWSSagemakerModel_primaryContainerHostname (23.82s)
--- PASS: TestAccAWSSagemakerModel_tags (32.58s)
--- PASS: TestAccAWSSagemakerModel_containers (33.86s)
--- PASS: TestAccAWSSagemakerModel_networkIsolation (35.85s)
--- PASS: TestAccAWSSagemakerModel_primaryContainerModelDataUrl (39.13s)
--- PASS: TestAccAWSSagemakerModel_vpcConfig (39.20s)
Provides a SageMaker model resource. | ||
--- | ||
|
||
# aws\_sagemaker\_model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backslashes in resource documentation titles were removed from the codebase awhile ago. Presumably this PR predates that cleanup.
image = "%s" | ||
} | ||
|
||
tags { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An equals sign will be required here in Terraform 0.12 and is backwards compatible with Terraform 0.11. For operators, this configuration update will be detected with the terraform 0.12upgrade
command.
tags = {
Config: testAccSagemakerModelConfigTags(rName, image), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckSagemakerModelExists("aws_sagemaker_model.foo"), | ||
resource.TestCheckResourceAttr("aws_sagemaker_model.foo", "tags.%", "1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check seems flakey, but only in CI. Passes fine locally. May be some sort of eventual consistency issue.
--- FAIL: TestAccAWSSagemakerModel_tags (17.66s)
testing.go:538: Step 0 error: Check failed: Check 2/3 error: aws_sagemaker_model.foo: Attribute 'tags.%' expected "1", got "0"
func TestAccAWSSagemakerModel_primaryContainerEnvironment(t *testing.T) { | ||
rName := acctest.RandString(10) | ||
|
||
resource.Test(t, resource.TestCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely well after this pull request was introduced, we switched the majority of the codebase acceptance testing from resource.Test()
to resource.ParallelTest()
func TestAccAWSSagemakerModel_containers(t *testing.T) { | ||
rName := acctest.RandString(10) | ||
|
||
resource.Test(t, resource.TestCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely well after this pull request was introduced, we switched the majority of the codebase acceptance testing from resource.Test()
to resource.ParallelTest()
--- | ||
layout: "aws" | ||
page_title: "AWS: sagemaker_model" | ||
sidebar_current: "docs-aws-resource-sagemaker-model" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation page is missing a sidebar link in website/aws.erb
<li<%= sidebar_current("docs-aws-resource-sagemaker-model") %>>
<a href="/docs/providers/aws/r/sagemaker_model.html">aws_sagemaker_model</a>
</li>
primary_container { | ||
image = "%s" | ||
|
||
environment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An equals sign will be required here in Terraform 0.12 and is backwards compatible with Terraform 0.11. For operators, this configuration update will be detected with the terraform 0.12upgrade
command.
environment = {
image = "%s" | ||
} | ||
|
||
tags { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An equals sign will be required here in Terraform 0.12 and is backwards compatible with Terraform 0.11. For operators, this configuration update will be detected with the terraform 0.12upgrade
command.
tags = {
Output from acceptance testing: ``` --- PASS: TestAccAWSSagemakerModel_primaryContainerEnvironment (23.40s) --- PASS: TestAccAWSSagemakerModel_basic (23.70s) --- PASS: TestAccAWSSagemakerModel_primaryContainerHostname (23.82s) --- PASS: TestAccAWSSagemakerModel_tags (32.58s) --- PASS: TestAccAWSSagemakerModel_containers (33.86s) --- PASS: TestAccAWSSagemakerModel_networkIsolation (35.85s) --- PASS: TestAccAWSSagemakerModel_primaryContainerModelDataUrl (39.13s) --- PASS: TestAccAWSSagemakerModel_vpcConfig (39.20s) ```
This has been released in version 1.58.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
@bflad @jckuester Is it intended that the field subnets in vpc_config is called subnets and not subnet_ids like in lambda_function and eks_cluster. Stumpled upon that because I searched for the fields in vpc_config and did not find them in the documentation so I checked how they are called in other resources. I guess it is not possible to change it now because it is already realeased, right? |
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! |
Add the resource
aws_sagemaker_model
for the newly announced service called SageMaker:Acceptance tests
Updates