-
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
sagemaker_domain add docker_settings & fix domain_settings update #35416
sagemaker_domain add docker_settings & fix domain_settings update #35416
Conversation
Community NoteVoting for Prioritization
For Submitters
|
Hi @DrFaust92, appreciate if you can review the PR and provide feedback. |
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.
Welcome @deepakbshetty 👋
It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.
Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.
Thanks again, and welcome to the community! 😃
8fa6223
to
d4b6a66
Compare
c3d4c88
to
08f6b90
Compare
99c168f
to
60c92fc
Compare
deepakbshetty looks great. lets split to a PR per resource please |
93ce950
to
3937519
Compare
Dear @DrFaust92 , Thanks for the speedy review. Split PRs to 3 - #35477 & #35479 |
a347f44
to
eb4db26
Compare
Thank you for your contribution! 🚀 Please note that typically Go dependency changes are handled in this repository by dependabot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the Additional details:
|
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.
LGTM (didnt validate tests)
42c09b1
to
3bac519
Compare
Hi @ewbankkit, If I may request this for merge. There is considerable upvote to include this feature |
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.
LGTM
Dear @ewbankkit, Any chance if you consider these sagemaker domain requests that have passed checks #35416 and #38457 and clears multiple issues |
33c8b9d
to
1479d8f
Compare
@ewbankkit For your kind consideration of both PRs for sagemaker domain. |
Hello @deepakbshetty , any ideas on resolving these conflicts so we can get this PR merged? Same with web portal settings PR |
Hi 👋 @jmeisele, These seem to have appeared due to PR aws sdk go v2 migration for sagemaker. I have a look this in the weekend. Would have been simpler if this code was merged prior. |
18861a8
to
803c64a
Compare
803c64a
to
d4621a8
Compare
Hi @jmeisele, Resolved merge conflicts and both PRs are now inline with aws-go-sdk-v2 changes. |
Hi @ewbankkit, If you can kindly consider PRs #35416 an #38457 for sagemaker_domain. Not having these settings block is causing us to fall back non terraform approach |
Hi @ewbankkit Is there any opportunity consider this long standing PR for current provider release please ? |
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.
LGTM 🚀.
% make testacc TESTARGS='-run=TestAccSageMaker_serial/Domain/basic\|TestAccSageMaker_serial/Domain/domainSettings' PKG=sagemaker
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.6 test ./internal/service/sagemaker/... -v -count 1 -parallel 20 -run=TestAccSageMaker_serial/Domain/basic\|TestAccSageMaker_serial/Domain/domainSettings -timeout 360m
=== RUN TestAccSageMaker_serial
=== PAUSE TestAccSageMaker_serial
=== CONT TestAccSageMaker_serial
=== RUN TestAccSageMaker_serial/Domain
=== RUN TestAccSageMaker_serial/Domain/basic
=== RUN TestAccSageMaker_serial/Domain/domainSettings
=== RUN TestAccSageMaker_serial/Domain/domainSettingsDockerSettingsUpdated
--- PASS: TestAccSageMaker_serial (745.01s)
--- PASS: TestAccSageMaker_serial/Domain (745.01s)
--- PASS: TestAccSageMaker_serial/Domain/basic (250.36s)
--- PASS: TestAccSageMaker_serial/Domain/domainSettings (262.40s)
--- PASS: TestAccSageMaker_serial/Domain/domainSettingsDockerSettingsUpdated (232.24s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/sagemaker 750.202s
@deepakbshetty Thanks for the contribution 🎉 👏. |
This functionality has been released in v5.67.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. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Description
Adds docker_settings to domain_settings under
aws_sagemaker_domain
resource.Add/Fix r_studio_server_pro_domain_settings update under domain_settings update to avoid drift.
Relations
Closes #35115
Fixes #31912
References
Create Domain API
Update Domain API
Output from Acceptance Testing