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

r/aws_connect_contact_flow - add new resource #16854

Merged
merged 14 commits into from
Sep 23, 2021

Conversation

thornleyk
Copy link
Contributor

@thornleyk thornleyk commented Dec 21, 2020

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #16392 #16854

Release note for CHANGELOG:

resource_aws_connect_contact_flow - add new resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsConnectContactFlow_'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsConnectContactFlow_ -timeout 120m
=== RUN   TestAccAwsConnectContactFlow_basic
=== PAUSE TestAccAwsConnectContactFlow_basic
=== RUN   TestAccAwsConnectContactFlow_filename
=== PAUSE TestAccAwsConnectContactFlow_filename
=== RUN   TestAccAwsConnectContactFlow_disappears
=== PAUSE TestAccAwsConnectContactFlow_disappears
=== CONT  TestAccAwsConnectContactFlow_basic
=== CONT  TestAccAwsConnectContactFlow_disappears
=== CONT  TestAccAwsConnectContactFlow_filename
--- PASS: TestAccAwsConnectContactFlow_disappears (108.34s)
--- PASS: TestAccAwsConnectContactFlow_basic (130.08s)
--- PASS: TestAccAwsConnectContactFlow_filename (153.91s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	154.043s
...
$ make testacc TESTARGS='-run=TestAccDataSourceAwsConnectContactFlow_'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsConnectContactFlow_ -timeout 120m
=== RUN   TestAccDataSourceAwsConnectContactFlow_basic
--- PASS: TestAccDataSourceAwsConnectContactFlow_basic (107.32s)
=== RUN   TestAccDataSourceAwsConnectContactFlow_byname
--- PASS: TestAccDataSourceAwsConnectContactFlow_byname (106.24s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	213.692s
...

@thornleyk thornleyk requested a review from a team as a code owner December 21, 2020 05:11
@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/connect Issues and PRs that pertain to the connect service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Dec 21, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Dec 21, 2020
@thornleyk thornleyk marked this pull request as draft December 21, 2020 07:24
@thornleyk thornleyk mentioned this pull request Dec 21, 2020
@thornleyk thornleyk marked this pull request as ready for review December 21, 2020 08:26
Base automatically changed from master to main January 23, 2021 01:00
@tms2018
Copy link

tms2018 commented May 29, 2021

Even though it doesn't strictly match the AWS API, it might be worth considering turning this into two separate resources: the contact flow and the flow actions. It's a common pattern for two contact flows to reference each other (eg. go back to menu). As currently implemented, this results in a cycle.

@thornleyk
Copy link
Contributor Author

Even though it doesn't strictly match the AWS API, it might be worth considering turning this into two separate resources: the contact flow and the flow actions. It's a common pattern for two contact flows to reference each other (eg. go back to menu). As currently implemented, this results in a cycle.

Hi, I'm unsure of what you mean by "flow actions" as a resource, this cyclic dependency will always be an issue, unless you think we run through contacts flows on initial create its an empty contact flow and then add the content? Do you have examples of where this is implemented as an AWS pattern in the provider?

@tms2018
Copy link

tms2018 commented May 30, 2021

By "flow actions" I mean the individual blocks which make up the flow (Disconnect, Play Prompt, etc.). In the JSON for a contact flow, they are in an array called "Actions".

I don't know of a place where something is implemented like this in the provider, as I've only started using Terraform due to client requirements and found this pull request during my research. But I have implemented a custom resource for the AWS CDK (which my company normally uses) to solve the same problem.

The only gotcha is, due to the API publishing the contact flow when it's created, any call to createContactFlow does have to have valid content for the flow. In our case, we use the minimum valid flow with a single Disconnect action. Then, as a second step, the flow is updated to the full flow. This allows the flow content to link to other flows, guaranteeing that the flows are created first and no cycles are created.

I anticipate it would look something like this

resource "aws_connect_contact_flow" "example" {
    instance_id = "aaaaaaaa-bbbb-cccc-dddd-111111111111"
    name        = "Test"
    description = "Test Contact Flow Description"
    type        = "CONTACT_FLOW"
    # content = file("flowFile")      # Possibly leave content here to optionally be filled if cycles aren't an issue  
}

resource "aws_connect_contact_flow_content" "example_content" {
    instance_id = "aaaaaaaa-bbbb-cccc-dddd-111111111111"
    contact_flow_id = aws_connect_contact_flow.example.id
    content = templatefile("flowFileWithDependencies", { otherFlowArn = aws_connect_contact_flow.other.arn })
}

I don't like that it requires the workaround of creating the initial minimal flow, but it's a limitation of the API that an empty flow can't be created. Not solving the cycle issue, though, makes it ultimately unusable in a lot of cases.

@thornleyk
Copy link
Contributor Author

By "flow actions" I mean the individual blocks which make up the flow (Disconnect, Play Prompt, etc.). In the JSON for a contact flow, they are in an array called "Actions".

I don't know of a place where something is implemented like this in the provider, as I've only started using Terraform due to client requirements and found this pull request during my research. But I have implemented a custom resource for the AWS CDK (which my company normally uses) to solve the same problem.

The only gotcha is, due to the API publishing the contact flow when it's created, any call to createContactFlow does have to have valid content for the flow. In our case, we use the minimum valid flow with a single Disconnect action. Then, as a second step, the flow is updated to the full flow. This allows the flow content to link to other flows, guaranteeing that the flows are created first and no cycles are created.

I anticipate it would look something like this

resource "aws_connect_contact_flow" "example" {
    instance_id = "aaaaaaaa-bbbb-cccc-dddd-111111111111"
    name        = "Test"
    description = "Test Contact Flow Description"
    type        = "CONTACT_FLOW"
    # content = file("flowFile")      # Possibly leave content here to optionally be filled if cycles aren't an issue  
}

resource "aws_connect_contact_flow_content" "example_content" {
    instance_id = "aaaaaaaa-bbbb-cccc-dddd-111111111111"
    contact_flow_id = aws_connect_contact_flow.example.id
    content = templatefile("flowFileWithDependencies", { otherFlowArn = aws_connect_contact_flow.other.arn })
}

I don't like that it requires the workaround of creating the initial minimal flow, but it's a limitation of the API that an empty flow can't be created. Not solving the cycle issue, though, makes it ultimately unusable in a lot of cases.

Thats going to be really difficult to synchronise the state to the remote terraform state as there is not resource id for logical parts of a contact flow definition so that your can remerge the individual actions back to an update request. I use the API at the moment in the grain defined here very effectively, it's just not managed in the terraform lifecycle, also I cannot easily reference lambda and lex resources managed by terraform, which is what this will allow

@AdamTylerLynch
Copy link
Collaborator

@thornleyk did you merge in the other PR from @abebars ? I see substantially similar code in your PR to theirs (#16709)

@thornleyk
Copy link
Contributor Author

@thornleyk did you merge in the other PR from @abebars ? I see substantially similar code in your PR to theirs (#16709)

Yeah I'm still waiting on terraform to merge @abebars original PR once that happens I'll rebase and finish this PR I'm dependent on that PR

@github-actions github-actions bot added the size/XL Managed by automation to categorize the size of a PR. label Jul 31, 2021
@AdamTylerLynch
Copy link
Collaborator

@thornleyk did you merge in the other PR from @abebars ? I see substantially similar code in your PR to theirs (#16709)

Yeah I'm still waiting on terraform to merge @abebars original PR once that happens I'll rebase and finish this PR I'm dependent on that PR

I'm going to fetch upstream/main and then port some of your changes into PR #16709.

@AdamTylerLynch
Copy link
Collaborator

tfproviderdocs linter error unrelated.

@breathingdust breathingdust added new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 8, 2021
@AdamTylerLynch
Copy link
Collaborator

make testacc TESTARGS='-run=TestAccDataSourceAwsConnectContact' ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 2 -run=TestAccDataSourceAwsConnectContact -timeout 180m
=== RUN   TestAccDataSourceAwsConnectContactFlow_basic
--- PASS: TestAccDataSourceAwsConnectContactFlow_basic (87.58s)
=== RUN   TestAccDataSourceAwsConnectContactFlow_byname
--- PASS: TestAccDataSourceAwsConnectContactFlow_byname (87.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	176.482s




make testacc TESTARGS='-run=TestAccAwsConnectContact' ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 2 -run=TestAccAwsConnectContact -timeout 180m
=== RUN   TestAccAwsConnectContactFlow_basic
=== PAUSE TestAccAwsConnectContactFlow_basic
=== RUN   TestAccAwsConnectContactFlow_filename
=== PAUSE TestAccAwsConnectContactFlow_filename
=== RUN   TestAccAwsConnectContactFlow_disappears
=== PAUSE TestAccAwsConnectContactFlow_disappears
=== CONT  TestAccAwsConnectContactFlow_basic
=== CONT  TestAccAwsConnectContactFlow_disappears
    resource_aws_connect_contact_flow_test.go:121: Step 1/1 error: Expected a non-empty plan, but got an empty plan
--- FAIL: TestAccAwsConnectContactFlow_disappears (86.15s)
=== CONT  TestAccAwsConnectContactFlow_filename
--- PASS: TestAccAwsConnectContactFlow_basic (109.16s)
--- PASS: TestAccAwsConnectContactFlow_filename (106.87s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	194.868s
FAIL
make: *** [testacc] Error 1

Need to finish the disappears test.

@AdamTylerLynch
Copy link
Collaborator

@thornleyk can you remove the WIP prefix?

@thornleyk thornleyk changed the title [WIP] r/aws_connect_contact_flow - add new resource r/aws_connect_contact_flow - add new resource Sep 20, 2021
@anGie44 anGie44 self-assigned this Sep 21, 2021
@anGie44 anGie44 added the new-data-source Introduces a new data source. label Sep 23, 2021
@anGie44
Copy link
Contributor

anGie44 commented Sep 23, 2021

Hi @thornleyk 👋 I'm giving this PR another pass before we merge. Overall it's looking great, just a couple small updates in the data source for now e.g. marking fields as Computed when not used as input.

@thornleyk
Copy link
Contributor Author

Hi @thornleyk 👋 I'm giving this PR another pass before we merge. Overall it's looking great, just a couple small updates in the data source for now e.g. marking fields as Computed when not used as input.

Thanks a bunch angie!!, I'll keep these updates in mind for the other resources

Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to this PR @thornleyk @AdamTylerLynch 🚀 ! I've added a couple finishing touches to the resource to get this into our upcoming release.

Output of acceptance tests:

--- PASS: TestAccAwsConnectContactFlowDataSource_Name (87.75s)
--- PASS: TestAccAwsConnectContactFlowDataSource_ContactFlowId (123.12s)

--- PASS: TestAccAwsConnectContactFlow_serial (0.00s)
    --- PASS: TestAccAwsConnectContactFlow_serial/basic (100.41s)
    --- PASS: TestAccAwsConnectContactFlow_serial/disappears (77.36s)
    --- PASS: TestAccAwsConnectContactFlow_serial/filename (101.17s)

@anGie44 anGie44 added this to the v3.60.0 milestone Sep 23, 2021
@anGie44 anGie44 merged commit 36596b8 into hashicorp:main Sep 23, 2021
@github-actions
Copy link

This functionality has been released in v3.60.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!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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/connect Issues and PRs that pertain to the connect service. size/XL Managed by automation to categorize the size of a PR. 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.

5 participants