Skip to content

Conversation

zhihaos
Copy link
Contributor

@zhihaos zhihaos commented Aug 6, 2025

fulfill request hashicorp/terraform-provider-google#23328

Previous PR: #14446

dialogflowcx: added `handlers`, `inputParameterDefinitions`, and `outputParameterDefinitions` to `google_dialogflow_cx_playbook` resource.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 43
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • dialogflowcx

🟢 All tests passed!

View the build log

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

google provider: Diff ( 4 files changed, 11396 insertions(+), 113 deletions(-))
google-beta provider: Diff ( 4 files changed, 11396 insertions(+), 113 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 3327 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 721 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 43
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • dialogflowcx

🟢 All tests passed!

View the build log

Copy link

@GoogleCloudPlatform/terraform-team @rileykarson This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

displayName, ok := res["displayName"].(string)
if !ok { return nil, fmt.Errorf("'displayName' is not a string") }

if displayName == "Playbook Example with Fulfillment" {
Copy link
Member

Choose a reason for hiding this comment

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

As discussed previously- this means that we're making changes that will only support the specific test cases, and mean the general fields/resource likely won't work for customers. Can you remove all test case-specific modifications (i.e. name checks, references to specific field indices unless they're a fixed part of the API contract we need to account for, etc) and push a commit with that change? That'll help me understand the base issue we're running into and suggest corrections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, these changes only support the specific test case, because other tests will fail due to not having the fields.

The specific test case is testing for optional fields for 100% test coverage and other tests do not use these optional fields so it will be a not found error for the others.

text = ["One more time?"]
}
}
messages {
Copy link
Member

Choose a reason for hiding this comment

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

The API response seems to drop these values entirely. We get back:

      "eventHandler": {
        "event": "sys.no-input-default",
        "fulfillment": {
          "messages": [
            {
              "text": {
                "text": [
"One more time?"
                ]
              }
            },
            {},
            {
              "knowledgeInfoCard": {}
            }
          ],

Is this intentional? That may be the case if those are all default values as proto<>json conversion collapses defaults into null generally. Or just a decision not to return those value(s) back. If not, is this a bug that will be fixed? Is it documented somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

(I suspect this line of inquiry will carry through the resource- removing the corrections in the decoder will highlight them all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is expected, this is why I added the decoder for this specific test case.

@@ -45,6 +46,17 @@ examples:
vars:
agent_name: 'dialogflowcx-agent'
bucket_name: 'dialogflowcx-bucket'
ignore_read_extra:
Copy link
Member

Choose a reason for hiding this comment

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

Let's omit these entries for now, and evaluate adding them back later if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just ran the test locally without the ignore_read_extra and this is the error I got:

resource_dialogflow_cx_playbook_generated_test.go:114: Step 2/2 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
        
          map[string]string{
        - 	"handlers.2.event_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.#":                          "1",
        - 	"handlers.2.event_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.0.%":                        "1",
        - 	"handlers.2.event_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.0.uri":                      "gs://tf-test-dialogflowcx-bucketrc419zoasl/prefix-",
        - 	"handlers.2.event_handler.0.fulfillment.0.messages.1.tool_call.0.action":                                               "Example Tool",
        + 	"handlers.2.event_handler.0.fulfillment.0.messages.1.tool_call.0.action":                                               "",
        - 	"handlers.2.event_handler.0.fulfillment.0.messages.1.tool_call.0.tool":                                                 "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"handlers.2.event_handler.0.fulfillment.0.messages.1.tool_call.0.tool":                                                 "",
        - 	"handlers.3.event_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.action":     "Example Tool",
        + 	"handlers.3.event_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.action":     "",
        - 	"handlers.3.event_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.tool":       "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"handlers.3.event_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.tool":       "",
        - 	"handlers.6.lifecycle_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.#":                      "1",
        - 	"handlers.6.lifecycle_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.0.%":                    "1",
        - 	"handlers.6.lifecycle_handler.0.fulfillment.0.advanced_settings.0.audio_export_gcs_destination.0.uri":                  "gs://tf-test-dialogflowcx-bucketrc419zoasl/prefix-",
        - 	"handlers.6.lifecycle_handler.0.fulfillment.0.messages.1.tool_call.0.action":                                           "Example Tool",
        + 	"handlers.6.lifecycle_handler.0.fulfillment.0.messages.1.tool_call.0.action":                                           "",
        - 	"handlers.6.lifecycle_handler.0.fulfillment.0.messages.1.tool_call.0.tool":                                             "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"handlers.6.lifecycle_handler.0.fulfillment.0.messages.1.tool_call.0.tool":                                             "",
        - 	"handlers.7.lifecycle_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.action": "Example Tool",
        + 	"handlers.7.lifecycle_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.action": "",
        - 	"handlers.7.lifecycle_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.tool":   "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"handlers.7.lifecycle_handler.0.fulfillment.0.conditional_cases.1.cases.0.case_content.8.message.0.tool_call.0.tool":   "",
        - 	"input_parameter_definitions.0.type_schema.0.schema_reference.0.tool":                                                  "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"input_parameter_definitions.0.type_schema.0.schema_reference.0.tool":                                                  "",
        - 	"output_parameter_definitions.0.type_schema.0.schema_reference.0.tool":                                                 "projects/zhihaos-grecords-retail-bot/locations/global/agents/817a04b9-a8b6-4e24-9ed2-9951b233f8f3/tools/c5206a3b-d62e-4a5a-a198-b23afcd7bb70",
        + 	"output_parameter_definitions.0.type_schema.0.schema_reference.0.tool":                                                 "",
          }

Copy link
Contributor Author

@zhihaos zhihaos Aug 26, 2025

Choose a reason for hiding this comment

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

I can omit the logging_settings in the ignore_read_extra and still pass the test, should I do that then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed logging_settings in the ignore_read_extra.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 26, 2025
@github-actions github-actions bot requested a review from rileykarson August 26, 2025 19:49
@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Aug 26, 2025
@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.

google provider: Diff ( 4 files changed, 11396 insertions(+), 113 deletions(-))
google-beta provider: Diff ( 4 files changed, 11396 insertions(+), 113 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 3327 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 721 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 43
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • dialogflowcx

🟢 All tests passed!

View the build log

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Aug 28, 2025
@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.

google provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
google-beta provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 3327 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 721 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 42
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • dialogflowcx

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link

@rileykarson This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Sep 2, 2025

@GoogleCloudPlatform/terraform-team @rileykarson This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Sep 9, 2025

@GoogleCloudPlatform/terraform-team @rileykarson This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

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

google provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
google-beta provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 3327 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 721 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 42
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • dialogflowcx

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link

@GoogleCloudPlatform/terraform-team @rileykarson This PR has been waiting for review for 3 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

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

google provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
google-beta provider: Diff ( 4 files changed, 10868 insertions(+), 110 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 3327 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 721 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 43
Passed tests: 42
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • dialogflowcx

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccDialogflowCXPlaybook_dialogflowcxPlaybookFulfillmentExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants