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

Adding dialogflow_cx_test_case resource #8879

Conversation

mmurakowski-verily
Copy link
Contributor

@mmurakowski-verily mmurakowski-verily commented Sep 6, 2023

Adds a a test_case resource. Fixes hashicorp/terraform-provider-google#15744

Release Note Template for Downstream PRs (will be copied)

`google_dialogflow_cx_test_case`

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 3066 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 3066 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 506 insertions(+))
TF OiCS: Diff ( 4 files changed, 229 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3016
Passed tests 2717
Skipped tests: 296
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample|TestAccDialogflowCXTestCase_update|TestAccFolderIamPolicy_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample[Debug log]
TestAccDialogflowCXTestCase_update[Debug log]
TestAccFolderIamPolicy_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@mmurakowski-verily mmurakowski-verily marked this pull request as ready for review September 7, 2023 00:33
@mmurakowski-verily
Copy link
Contributor Author

mmurakowski-verily commented Sep 7, 2023

It's the API, but I'm iffy about if it's worthwhile to have currentPage.displayName and triggeredIntent.displayName: if you have both, you end up just repeating yourself in whatever terraform you write, and if you don't want to repeat yourself, terraform apply will keep trying to remove the missing one after the resource is created:

  # google_dialogflow_cx_test_case.basic_test_case will be updated in-place
  ~ resource "google_dialogflow_cx_test_case" "basic_test_case" {
        id               = "REDACTED"
        name             = "REDACTED"
        tags             = [
            "#tag1",
        ]
        # (5 unchanged attributes hidden)

      ~ test_case_conversation_turns {
          ~ virtual_agent_output {
                # (1 unchanged attribute hidden)

              ~ current_page {
                  - display_name = "MyPage" -> null
                    name         = "REDACTED"
                }

              ~ triggered_intent {
                  - display_name = "MyIntent" -> null
                    name         = "REDACTED"
                }

And thinking about it more, I also don't love

triggered_intent {
  name = google_dialogflow_cx_intent.some_intent.id
}
current_page {
  name = google_dialogflow_cx_page.some_page.id
}

It'd be nicer as

triggered_intent = google_dialogflow_cx_intent.some_intent.id
current_page = google_dialogflow_cx_page.some_page.id

but that's moving even further away from the API and feels like it's inviting a future breaking change. WDYT?

@melinath
Copy link
Member

melinath commented Sep 8, 2023

In terms of "flattening" nested fields to something like:

triggered_intent = google_dialogflow_cx_intent.some_intent.id
current_page = google_dialogflow_cx_page.some_page.id

We don't allow that in the provider any more unless it's to match an existing implementation (for example, a sibling resource that was implemented that way), because there have been too many cases where other nested fields ended up being added later and causing confusion / bugs / extra work.


Regarding the display_name fields - if the API requires them to always be equal to the google_dialogflow_cx_intent.display_name and google_dialogflow_cx_page.display_name fields, then it's probably fine to not add them to the resource for now (or make them output-only). Folks can just reference the value from the relevant resource directly. If there's a valid reason you're aware of that it would be different than the intent/page resource's display_name, then implementing it would be preferable.

@mmurakowski-verily
Copy link
Contributor Author

too many cases where other nested fields ended up being added later and causing confusion / bugs / extra work

Yeah, I figured; oh well.

I tested it and it looks like if you provide a display_name and name that don't match, name wins; it picks the Intent with the ID (name) you gave it, and uses that one's display_name as the triggered_intent.display_name. I made the display_name output only and added a comment so hopefully nobody gets confused in the future.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 3057 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 3057 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 506 insertions(+))
TF OiCS: Diff ( 4 files changed, 226 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3022
Passed tests 2722
Skipped tests: 297
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample|TestAccDialogflowCXTestCase_update|TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample[Debug log]
TestAccDialogflowCXTestCase_update[Debug log]
TestAccSpannerDatabaseIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

A few small changes & questions - this is looking really good

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 3053 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 3058 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 506 insertions(+))
TF OiCS: Diff ( 4 files changed, 226 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3028
Passed tests 2731
Skipped tests: 297
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@mmurakowski-verily mmurakowski-verily force-pushed the adding_resource_dialogflow_cx_test_case branch from d9c185c to 79f6078 Compare September 11, 2023 18:55
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 3053 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 3053 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 506 insertions(+))
TF OiCS: Diff ( 4 files changed, 226 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3028
Passed tests 2727
Skipped tests: 297
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDialogflowCXTestCase_update|TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccFolderIamPolicy_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDialogflowCXTestCase_update[Debug log]
TestAccDialogflowCXTestCase_dialogflowcxTestCaseFullExample[Debug log]
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Debug log]
TestAccFolderIamPolicy_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

LGTM

RileyHYZ pushed a commit to RileyHYZ/magic-modules that referenced this pull request Sep 15, 2023
* Adding dialogflow_cx_test_case resource

* Making displayName output-only

* Using %parent instead of custom import code

* Adding comment clarifying why intent + audio are missing

* Making tests exercise updating more fields
joelkattapuram pushed a commit to joelkattapuram/magic-modules that referenced this pull request Sep 20, 2023
* Adding dialogflow_cx_test_case resource

* Making displayName output-only

* Using %parent instead of custom import code

* Adding comment clarifying why intent + audio are missing

* Making tests exercise updating more fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Dialogflow CX Test Case resources
3 participants