Skip to content
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

Merged
merged 5 commits into from
Aug 28, 2020
Merged

Add Lex slot type data source and resource #8916

merged 5 commits into from
Aug 28, 2020

Conversation

jzbruno
Copy link
Contributor

@jzbruno jzbruno commented Jun 10, 2019

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

Relates #905 and split from PR #2616

Release note for CHANGELOG:

FEATURES:
* **New Data Source:** `aws_lex_slot_type`
* **New Resource:** `aws_lex_slot_type`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAcc.*Lex.*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAcc.*Lex.* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccDataSourceAwsLexSlotType
--- PASS: TestAccDataSourceAwsLexSlotType (12.52s)
=== RUN   TestAccLexSlotType_basic
=== PAUSE TestAccLexSlotType_basic
=== RUN   TestAccAwsLexSlotType_CreateVersion
=== PAUSE TestAccAwsLexSlotType_CreateVersion
=== RUN   TestAccAwsLexSlotType_Description
=== PAUSE TestAccAwsLexSlotType_Description
=== RUN   TestAccAwsLexSlotType_EnumerationValue
=== PAUSE TestAccAwsLexSlotType_EnumerationValue
=== RUN   TestAccAwsLexSlotType_Name
=== PAUSE TestAccAwsLexSlotType_Name
=== RUN   TestAccAwsLexSlotType_ValueSelectionStrategy
=== PAUSE TestAccAwsLexSlotType_ValueSelectionStrategy
=== CONT  TestAccLexSlotType_basic
=== CONT  TestAccAwsLexSlotType_Name
=== CONT  TestAccAwsLexSlotType_ValueSelectionStrategy
=== CONT  TestAccAwsLexSlotType_EnumerationValue
=== CONT  TestAccAwsLexSlotType_Description
=== CONT  TestAccAwsLexSlotType_CreateVersion
--- PASS: TestAccLexSlotType_basic (9.37s)
--- PASS: TestAccAwsLexSlotType_CreateVersion (18.77s)
--- PASS: TestAccAwsLexSlotType_Description (19.74s)
--- PASS: TestAccAwsLexSlotType_EnumerationValue (21.03s)
--- PASS: TestAccAwsLexSlotType_Name (22.03s)
--- PASS: TestAccAwsLexSlotType_ValueSelectionStrategy (23.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	35.933s
...

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/lexmodels Issues and PRs that pertain to the lexmodels service. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 10, 2019
@bflad bflad added new-data-source Introduces a new data source. new-resource Introduces a new resource. labels Jun 10, 2019
@bflad bflad added this to the v2.15.0 milestone Jun 10, 2019
@bflad bflad self-assigned this Jun 10, 2019
@nywilken nywilken modified the milestones: v2.15.0, v2.16.0 Jun 10, 2019
@bflad bflad assigned nywilken and unassigned bflad Jun 10, 2019
Copy link
Member

@nywilken nywilken left a 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.

aws/data_source_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/validators_lex_constants.go Outdated Show resolved Hide resolved
aws/resource_aws_lex.go Outdated Show resolved Hide resolved
aws/resource_aws_lex.go Outdated Show resolved Hide resolved
aws/resource_aws_lex.go Outdated Show resolved Hide resolved
website/docs/r/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 12, 2019

@nywilken Except for the question above, I have made the requested changes to all 4 PRs.

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 13, 2019

@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.

Copy link
Member

@nywilken nywilken left a 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.

aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/resource_aws_lex.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Show resolved Hide resolved
aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 14, 2019

@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?

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 14, 2019

@nywilken
Will wait on version until we discuss further. I updated the PR with the other suggestions.

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.

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 15, 2019

@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.

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 15, 2019

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?

Copy link
Member

@nywilken nywilken left a 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.

aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
website/docs/r/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
website/docs/r/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
@nywilken nywilken modified the milestones: v2.16.0, v2.17.0 Jun 20, 2019
@nywilken nywilken added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 20, 2019
@nywilken nywilken modified the milestones: v2.17.0, v2.18.0 Jun 21, 2019
@jzbruno jzbruno requested a review from a team June 23, 2019 15:50
@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 23, 2019

@nywilken Ok. Updated for your comments. This should be good for another review. Thanks. And I will start work on the other PRs now.

@gdavison
Copy link
Contributor

The dependency check is flagging the updates to go.mod, go.sum, and vendor/. Since this PR has been around for a while, you probably had to make a number of updates. The easiest way to work around this would be to either rebase your branch on the current main branch or to merge the current main branch into your PR. Github will see the changes either way. You may also need to run go mod tidy once or twice to remove changes to go.mod

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 29, 2020

Ok, thanks. I will try to resolve that and finish the other 2 comments tonight. Sorry for the delay on this.

@jzbruno
Copy link
Contributor Author

jzbruno commented Jun 30, 2020

@gdavison I believe I have addressed all of your comments. Latest acc test run shows

~/src/github/terraform-providers/terraform-provider-aws [error 2] $ make testacc TEST=./aws TESTARGS='-run=TestAcc.*AwsLex.*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAcc.*AwsLex.* -timeout 120m
=== RUN   TestAccDataSourceAwsLexSlotType_basic
--- PASS: TestAccDataSourceAwsLexSlotType_basic (19.78s)
=== RUN   TestAccDataSourceAwsLexSlotType_withVersion
--- PASS: TestAccDataSourceAwsLexSlotType_withVersion (20.10s)
=== RUN   TestAccAwsLexSlotType_createVersion
=== PAUSE TestAccAwsLexSlotType_createVersion
=== RUN   TestAccAwsLexSlotType_description
=== PAUSE TestAccAwsLexSlotType_description
=== RUN   TestAccAwsLexSlotType_enumerationValues
=== PAUSE TestAccAwsLexSlotType_enumerationValues
=== RUN   TestAccAwsLexSlotType_name
=== PAUSE TestAccAwsLexSlotType_name
=== RUN   TestAccAwsLexSlotType_valueSelectionStrategy
=== PAUSE TestAccAwsLexSlotType_valueSelectionStrategy
=== RUN   TestAccAwsLexSlotType_disappears
=== PAUSE TestAccAwsLexSlotType_disappears
=== CONT  TestAccAwsLexSlotType_createVersion
=== CONT  TestAccAwsLexSlotType_valueSelectionStrategy
=== CONT  TestAccAwsLexSlotType_enumerationValues
=== CONT  TestAccAwsLexSlotType_name
=== CONT  TestAccAwsLexSlotType_disappears
=== CONT  TestAccAwsLexSlotType_description
--- PASS: TestAccAwsLexSlotType_disappears (15.02s)
--- PASS: TestAccAwsLexSlotType_description (33.38s)
--- PASS: TestAccAwsLexSlotType_createVersion (34.39s)
--- PASS: TestAccAwsLexSlotType_enumerationValues (35.59s)
--- PASS: TestAccAwsLexSlotType_valueSelectionStrategy (36.26s)
--- PASS: TestAccAwsLexSlotType_name (45.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	87.191s

Copy link
Contributor

@gdavison gdavison left a 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.

aws/data_source_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_lex_slot_type_test.go Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type.go Outdated Show resolved Hide resolved
aws/resource_aws_lex_slot_type_test.go Outdated Show resolved Hide resolved
website/docs/r/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
website/docs/r/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
website/docs/d/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
website/docs/d/lex_slot_type.html.markdown Outdated Show resolved Hide resolved
@jzbruno
Copy link
Contributor Author

jzbruno commented Jul 6, 2020

Working through these. Will finish up tonight.

@jzbruno
Copy link
Contributor Author

jzbruno commented Jul 7, 2020

Addressed most comments but had a couple questions. Latest acceptance test results.

~/src/github/terraform-providers/terraform-provider-aws $ make testacc TEST=./aws TESTARGS='-run=TestAcc.*AwsLexSlotType.*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAcc.*AwsLexSlotType.* -timeout 120m
=== RUN   TestAccDataSourceAwsLexSlotType_basic
=== PAUSE TestAccDataSourceAwsLexSlotType_basic
=== RUN   TestAccDataSourceAwsLexSlotType_withVersion
=== PAUSE TestAccDataSourceAwsLexSlotType_withVersion
=== RUN   TestAccAwsLexSlotType_basic
=== PAUSE TestAccAwsLexSlotType_basic
=== RUN   TestAccAwsLexSlotType_createVersion
=== PAUSE TestAccAwsLexSlotType_createVersion
=== RUN   TestAccAwsLexSlotType_description
=== PAUSE TestAccAwsLexSlotType_description
=== RUN   TestAccAwsLexSlotType_enumerationValues
=== PAUSE TestAccAwsLexSlotType_enumerationValues
=== RUN   TestAccAwsLexSlotType_name
=== PAUSE TestAccAwsLexSlotType_name
=== RUN   TestAccAwsLexSlotType_valueSelectionStrategy
=== PAUSE TestAccAwsLexSlotType_valueSelectionStrategy
=== RUN   TestAccAwsLexSlotType_disappears
=== PAUSE TestAccAwsLexSlotType_disappears
=== CONT  TestAccDataSourceAwsLexSlotType_basic
=== CONT  TestAccAwsLexSlotType_enumerationValues
=== CONT  TestAccAwsLexSlotType_createVersion
=== CONT  TestAccAwsLexSlotType_name
=== CONT  TestAccAwsLexSlotType_basic
=== CONT  TestAccAwsLexSlotType_valueSelectionStrategy
=== CONT  TestAccAwsLexSlotType_disappears
=== CONT  TestAccAwsLexSlotType_description
=== CONT  TestAccDataSourceAwsLexSlotType_withVersion
--- PASS: TestAccAwsLexSlotType_disappears (14.57s)
--- PASS: TestAccAwsLexSlotType_basic (19.99s)
--- PASS: TestAccDataSourceAwsLexSlotType_basic (20.33s)
--- PASS: TestAccDataSourceAwsLexSlotType_withVersion (26.15s)
--- PASS: TestAccAwsLexSlotType_enumerationValues (34.64s)
--- PASS: TestAccAwsLexSlotType_createVersion (35.31s)
--- PASS: TestAccAwsLexSlotType_description (35.99s)
--- PASS: TestAccAwsLexSlotType_valueSelectionStrategy (36.90s)
--- PASS: TestAccAwsLexSlotType_name (42.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	43.845s

Copy link
Contributor

@gdavison gdavison left a 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

aws/resource_aws_lex_slot_type.go Outdated Show resolved Hide resolved
@gdavison gdavison added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 11, 2020
@jzbruno
Copy link
Contributor Author

jzbruno commented Aug 13, 2020

@gdavison Thanks I will take a look this weekend.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 13, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

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 ("github.com/hashicorp/terraform-plugin-sdk/helper/schema"). Please import the V2 path github.com/hashicorp/terraform-plugin-sdk/v2/helper/PACKAGE

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.

@ghost
Copy link

ghost commented Aug 21, 2020

Hello, and thank you for your contribution!
This pull request appears to include the Go import path "github.com/hashicorp/terraform-plugin-sdk/helper/mutexkv", which was deprecated after upgrading to V2 of the Terraform Plugin SDK.
You may use a now internalized version of the package found in github.com/terraform-providers/terraform-provider-aws/aws/internal/PACKAGE.

@jzbruno
Copy link
Contributor Author

jzbruno commented Aug 21, 2020

@gdavison I think I finished all your suggestions.

Copy link
Contributor

@gdavison gdavison left a 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

@gdavison gdavison modified the milestones: Roadmap, v3.5.0 Aug 28, 2020
@gdavison gdavison merged commit 6302bf6 into hashicorp:master Aug 28, 2020
gdavison added a commit that referenced this pull request Aug 28, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

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!

@leonfs
Copy link

leonfs commented Sep 7, 2020

This is great news. @jzbruno Great work on getting the Lex Slot merged and deployed. Hopefully, the other 3 PRs will progress smoothly too.

New Resource: aws_lex_intent #8917
New Resource: aws_lex_bot #8918
New Resource: aws_lex_bot_alias #8919

@ghost
Copy link

ghost commented Sep 28, 2020

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!

@ghost ghost locked and limited conversation to collaborators Sep 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Used to indicate dependency changes. documentation Introduces or discusses updates to documentation. new-data-source Introduces a new data source. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/lexmodels Issues and PRs that pertain to the lexmodels service. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants