-
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_lex_resources #2616
Conversation
Hi @jzbruno Do you mind raising a separate PR for all the |
Sure. I'll break the vendoring out into it's own PR. |
@radeksimko I have created pull request #2650 to add vendoring for Lex. |
any update on this? It'd be such a help to have the lex resources available in the default binary! Thanks for working on this @jzbruno |
@giancarlopetrini I will review what is left to finish after work and work on it this weekend. |
Looks like I have the framework for most resources done but need to finish code, tests, and documentation. I will see what I can get done on Saturday. Question: This is going to be a fairly large addition, should I break it up into a few PRs? |
Generally speaking we prefer 1 resource/data source per PR just to make the reviewing process easier. Also since Lex is a new service, if you want make the |
Ok. I will break it up. There will end up being 4 resources.
With aws_lex_intent being the most complex. |
Ok. This is ready for review. I will work on the other resources tonight and create PRs for each of them.
|
Any reason this is not merged yet? |
Hi @jzbruno, Any update on this? We'd really like to use these resources. Cheers, |
I need to finish aws_lex_intent. The others have PRs and are waiting for review. If I remember right they should be reviewed in order. |
We'd love to see this released soon. Any news please? |
For whoever is going to review this. Would it be easier if I combined the PRs? And I will work on finishing the intent resource this weekend. |
+1 |
I have updated this branch to match the contribution guidelines and updated with the latest master branch. I will work on breaking apart the PR next. |
The Lex slot type resource and data source PR #8916 is ready for review. I am going to try and keep each PR independent and just deal with merge conflicts as each one is merged. |
@nywilken All PRs are ready for review. They can be reviewed independently but it would be best if they are merged in order. |
Great diligence @jzbruno! Thanks for this. Can't wait to see it get merged and released! |
@jzbruno thanks for the quick turnaround with splitting this into multiple PRs. While I can't give an actual time commitment I can say that this PR along with the newly created PRs are being prioritized for the v2.16.0 release. I myself am new to the Lex service so I will need a little time to get up to speed here, but the other maintainers are available to assist as well so please don't be surprised if you see comments and suggestions from multiple maintainers. I see you've called out the dependencies around testing for the new PRs. If there are any other edge cases or relevant information that comes to mind as we work through these PRs please feel free to add them as comments. Lastly, thanks for sticking with us and for pushing this contribution forward. |
@nywilken Sounds good. Thanks for your help getting this reviewed. |
Nearly there? |
Getting close. We are working through the review on the first PR. The rest should go pretty quick once that is finished as I am updating the other PRs with the comments from the first as we go. Those comments are happening here if you want to follow along / suggest anything. #8916 |
Since this pull request has been split into new ones (thank you!) (#2616 (comment)), closing this pull request as those should be followed instead.
For overall tracking of Lex support, please follow #905. 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! |
Fixes Issue #905