feature(sagemaker notebook): Add DirectInternetAccess support #8618

bcatubig · 2019-05-13T18:03:39Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #0000

Release note for CHANGELOG:

* Add direct_internet_access option to Sagemaker Notebook Resource

Output from acceptance testing:

~~I'm not able to run tests locally, would need some help with my local go setup~~

I'm not able to run tests. I will need these to be run by another maintainer.

⌂681% [brandon:~/go … terraform-provider-aws] feature/sagemaker-private-internet-access 1m7s ± make testacc TESTARGS='-run=TestAccAWSSagemakerNotebookInstance_directInternetAccess'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSSagemakerNotebookInstance_directInternetAccess -timeout 120m
main.go:5:2: cannot find package "github.com/terraform-providers/terraform-provider-aws/aws" in any of:
	/Users/brandon/go/src/github.com/bcatubig/terraform-provider-aws/vendor/github.com/terraform-providers/terraform-provider-aws/aws (vendor tree)
	/usr/local/Cellar/go/1.12.1/libexec/src/github.com/terraform-providers/terraform-provider-aws/aws (from $GOROOT)
	/Users/brandon/go/src/github.com/terraform-providers/terraform-provider-aws/aws (from $GOPATH)
make: *** [testacc] Error 1

bflad · 2019-05-15T13:04:16Z

Hi @bcatubig 👋 Is this pull request still work in progress as noted by the title?

Regarding the Go error you're seeing with the acceptance testing, you may find it easiest to move the source code directory outside your GOPATH since we've moved this codebase to using Go Modules. 👍

bcatubig · 2019-05-15T17:24:05Z

Hey @bflad -- this is ready for review/feedback. I borked my last PR for this feature by mistake.

I'll try moving the folder

rainmanh · 2019-06-04T06:10:27Z

@bcatubig @bflad, any estimate when this will be released please?

seoane · 2019-07-16T14:45:46Z

@bflad Greetings, do we have any eta for when this will be released?.
Thank you for everything!

pserrano · 2019-10-25T14:48:42Z

Hi,
when this feature will be released? It's very useful because if you need to disable the DirectInternetAccess you can't use terraform to handle this Notebook config
and you should do manually. Any ETA? @ryndaniels @bcatubig

bcatubig · 2019-10-25T14:51:20Z

I'll update this branch with upstream this weekend

pserrano · 2019-10-31T09:29:59Z

Any ETA about that?

This commit adds direct_internet_access property to SageMaker resource

bcatubig · 2019-11-12T03:45:55Z

PR has been re-synced with upstream. Commits have also been squashed to a single commit.

jacoor · 2019-12-10T16:29:29Z

Any update on state of this PR? Would be very usefull.

bflad

It looks like additional merge conflicts had crept in since the last rebase due to some unrelated tagging work being done across many of the resources in the provider. Rather than further delay the introduction of this functionality, we opted to fix those and fix remaining issues as noted below. Thanks for this contribution @bcatubig 🚀

Output from acceptance testing (after adjustments):

--- PASS: TestAccAWSSagemakerNotebookInstance_LifecycleConfigName (309.31s)
--- PASS: TestAccAWSSagemakerNotebookInstance_basic (310.81s)
--- PASS: TestAccAWSSagemakerNotebookInstance_disappears (315.88s)
--- PASS: TestAccAWSSagemakerNotebookInstance_tags (318.47s)
--- PASS: TestAccAWSSagemakerNotebookInstance_update (500.90s)
--- PASS: TestAccAWSSagemakerNotebookInstance_direct_internet_access (613.14s)

bflad · 2020-01-29T15:13:30Z

aws/resource_aws_sagemaker_notebook_instance_test.go

+
+resource "aws_subnet" "sagemaker" {
+  vpc_id     = "${aws_vpc.test.id}"
+  cidr_block = "10.0.0.0/27"


This test configuration is currently failing the new testing:

--- FAIL: TestAccAWSSagemakerNotebookInstance_direct_internet_access (19.44s) testing.go:640: Step 0 error: errors during apply: Error: Error creating subnet: InvalidSubnet.Range: The CIDR '10.0.0.0/27' is invalid.

After updating this argument, the testing was still failing with:

--- FAIL: TestAccAWSSagemakerNotebookInstance_direct_internet_access (21.87s) testing.go:640: Step 0 error: errors during apply: Error: Error creating Sagemaker Notebook Instance: ValidationException: Check if the security groups are specified

Adding an aws_security_group resource and setting security_groups in the aws_sagemaker_notebook_instance configuration fixed the issue. We will note this requirement in the documentation as well.

bflad · 2020-01-29T15:19:20Z

aws/resource_aws_sagemaker_notebook_instance.go

@@ -72,6 +73,16 @@ func resourceAwsSagemakerNotebookInstance() *schema.Resource {
 				ForceNew: true,
 			},

+			"direct_internet_access": {


Since the API defaults this attribute to Enabled, this schema also needs Default: sagemaker.DirectInternetAccessEnabled to prevent Terraform from recreating resources which do not have it defined:

--- FAIL: TestAccAWSSagemakerNotebookInstance_basic (301.98s) testing.go:640: Step 0 error: After applying this step, the plan was not empty: DIFF: DESTROY/CREATE: aws_sagemaker_notebook_instance.foo ... direct_internet_access: "Enabled" => "" (forces new resource) ...

…rect_internet_access PR feedback Reference: #8618 Output from acceptance testing: ``` --- PASS: TestAccAWSSagemakerNotebookInstance_LifecycleConfigName (309.31s) --- PASS: TestAccAWSSagemakerNotebookInstance_basic (310.81s) --- PASS: TestAccAWSSagemakerNotebookInstance_disappears (315.88s) --- PASS: TestAccAWSSagemakerNotebookInstance_tags (318.47s) --- PASS: TestAccAWSSagemakerNotebookInstance_update (500.90s) --- PASS: TestAccAWSSagemakerNotebookInstance_direct_internet_access (613.14s) ```

ghost · 2020-01-30T21:45:13Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-03-27T17:29:04Z

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!

bcatubig mentioned this pull request May 13, 2019

Add SageMaker support #2493

Closed

bflad added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels May 15, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label May 15, 2019

bcatubig mentioned this pull request Jun 17, 2019

[WIP] Sagemaker Notebook DirectInternetAccess support #7884

Closed

aeschright requested a review from a team June 26, 2019 16:53

nywilken requested a review from ryndaniels July 16, 2019 17:40

bcatubig changed the title ~~[WIP] feature(sagemaker notebook): Add DirectInternetAccess support~~ feature(sagemaker notebook): Add DirectInternetAccess support Jul 23, 2019

feature(sagemaker): Add direct_internet_access

da02cf0

This commit adds direct_internet_access property to SageMaker resource

bflad reviewed Jan 29, 2020

View reviewed changes

bflad added this to the v2.47.0 milestone Jan 29, 2020

bflad merged commit da02cf0 into hashicorp:master Jan 29, 2020

bflad added a commit that referenced this pull request Jan 29, 2020

Update CHANGELOG for #8618

7568581

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(sagemaker notebook): Add DirectInternetAccess support #8618

feature(sagemaker notebook): Add DirectInternetAccess support #8618

bcatubig commented May 13, 2019 •

edited

Loading

bflad commented May 15, 2019 •

edited

Loading

bcatubig commented May 15, 2019

rainmanh commented Jun 4, 2019

seoane commented Jul 16, 2019

pserrano commented Oct 25, 2019 •

edited

Loading

bcatubig commented Oct 25, 2019

pserrano commented Oct 31, 2019

bcatubig commented Nov 12, 2019

jacoor commented Dec 10, 2019

bflad left a comment

bflad Jan 29, 2020

bflad Jan 29, 2020

ghost commented Jan 30, 2020

ghost commented Mar 27, 2020

feature(sagemaker notebook): Add DirectInternetAccess support #8618

feature(sagemaker notebook): Add DirectInternetAccess support #8618

Conversation

bcatubig commented May 13, 2019 • edited Loading

Community Note

bflad commented May 15, 2019 • edited Loading

bcatubig commented May 15, 2019

rainmanh commented Jun 4, 2019

seoane commented Jul 16, 2019

pserrano commented Oct 25, 2019 • edited Loading

bcatubig commented Oct 25, 2019

pserrano commented Oct 31, 2019

bcatubig commented Nov 12, 2019

jacoor commented Dec 10, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad Jan 29, 2020

Choose a reason for hiding this comment

bflad Jan 29, 2020

Choose a reason for hiding this comment

ghost commented Jan 30, 2020

ghost commented Mar 27, 2020

bcatubig commented May 13, 2019 •

edited

Loading

bflad commented May 15, 2019 •

edited

Loading

pserrano commented Oct 25, 2019 •

edited

Loading