-
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
Add Lex slot type data source and resource #8916
Add Lex slot type data source and resource #8916
Conversation
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.
@jzbruno thanks again for pushing this forward. These two resources are looking pretty good. Nicely done.
There are a few comments that I would like for you to address before I move onto the next PR. Please let me know if you have any questions or additional thoughts on the requested changes. Cheers.
@nywilken Except for the question above, I have made the requested changes to all 4 PRs. |
@nywilken Ok. I have moved all common code into the resource that first uses them, and we will deal with the conflicts as each are merged. Waiting on response about moving expandLexSet into each of the 13 calls to it. |
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.
@jzbruno thanks for you patience on the review, I had a little more time to dive into the API and get a sense of what is expected for this resource. I left a few implementation suggestions to address your questions, and to improve the Acceptance testing.
There is a question around the need for the version attribute within the schema which from my testing always appears to be set to $LATEST
. Please take a look and let me know if you have any questions. I will start to review the other PRs now that I have a sense of what is happening with this service and this one is just about to be done.
Thanks again for your patience and support in pushing this service forward.
@nywilken So, version doesn't always have to be $LATEST, you could for example manage multiple versions of bots, intents, and slot types. Let's say you create v1 and then roll out v2 but v1 is still active you would still want to manage both. Another example would be having a dev and prod version. This does highlight that import is not handling that correctly. https://docs.aws.amazon.com/lex/latest/dg/versioning-aliases.html What do you think? |
@nywilken How far do we want to go with breaking apart the tests for the other resources? For the Intent resource there are 56 different update combinations due to the multiple levels of nested resources. |
@nywilken After reviewing the version issue further I think you are correct. The SlotType resource should always track the latest version so we can remove that as an input. I will add the create_version attribute to allow enable/disable of the version snapshotting for each change. And also add a resource for SlotTypeVersion to allow management of those version snapshots. |
Ok. This is ready for review again. Should handle version correctly. Only issue is dealing with attributes that are used for create / update but don't get returned from the API like create_version. They cause issues with importverifystate testing since the config isn't loaded at that stage and read can't get it from the API. Makes it so we have to disable the import verify on that particular test case. Any suggestions there? |
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.
@jzbruno thanks for making the updates. This looks good! I addressed your question on how the ImportVerifyState step should looks for virtual arguments (i.e arguments that passed to the API, but are not part of the API response) to get the test to pass. I also left a few more requested changes to call this done.
@nywilken Ok. Updated for your comments. This should be good for another review. Thanks. And I will start work on the other PRs now. |
The dependency check is flagging the updates to |
Ok, thanks. I will try to resolve that and finish the other 2 comments tonight. Sorry for the delay on this. |
@gdavison I believe I have addressed all of your comments. Latest acc test run shows
|
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.
Thanks, @jzbruno, it's looking good. I have a number of suggested changes. I ran into some errors when testing, specifically the TestAccLexSlotType_basic()
test.
Working through these. Will finish up tonight. |
Addressed most comments but had a couple questions. Latest acceptance test results.
|
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 is looking great, @jzbruno. A couple changes, including the multiple slot versions, and we can get it merged in.
You'll also need to update the version of the Terraform Plugin SDK to v2. Update references to the package github.com/hashicorp/terraform-plugin-sdk/*
to github.com/hashicorp/terraform-plugin-sdk/v2/*
. You may be able to Git rebase your branch against the current master branch (example below); replacing any remaining old import paths with the newer ones.
$ git fetch --all
$ git rebase origin/master
@gdavison Thanks I will take a look this weekend. |
Hello, and thank you for your contribution! This project recently upgraded to V2 of the Terraform Plugin SDK This pull request appears to include at least one V1 import path of the SDK ( To resolve this situation without losing any existing work, you may be able to Git rebase your branch against the current master branch (example below); replacing any remaining old import paths with the newer ones. $ git fetch --all
$ git rebase origin/master Another option is to create a new branch from the current master with the same code changes (replacing the import paths), submit a new pull request, and close this existing pull request. We apologize for this inconvenience and appreciate your effort. Thank you for contributing and helping make the Terraform AWS Provider better for everyone. |
Hello, and thank you for your contribution! |
@gdavison I think I finished all your suggestions. |
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 🚀
Acceptance tests in Commercial partitions
--- PASS: TestAccAwsLexSlotType_disappears (24.29s)
--- PASS: TestAccDataSourceAwsLexSlotType_withVersion (27.80s)
--- PASS: TestAccAwsLexSlotType_basic (30.06s)
--- PASS: TestAccDataSourceAwsLexSlotType_basic (37.82s)
--- PASS: TestAccAwsLexSlotType_valueSelectionStrategy (44.35s)
--- PASS: TestAccAwsLexSlotType_createVersion (45.07s)
--- PASS: TestAccAwsLexSlotType_description (45.85s)
--- PASS: TestAccAwsLexSlotType_enumerationValues (47.16s)
--- PASS: TestAccAwsLexSlotType_name (47.87s)
Not supported in GovCloud partitions. #14893 captures requirement for preCheck to skip acceptance tests
This has been released in version 3.5.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! |
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! |
Community Note
Relates #905 and split from PR #2616
Release note for CHANGELOG:
Output from acceptance testing: