Enabling management of default Dialogflow resources without terraform import: is_default_x fields

[mmurakowski-verily]

Fixes hashicorp/terraform-provider-google#16308, read that bug for more context 😅

An alternative version of #9359 because designing a good API is hard and I want someone else to make that decision 😅.

Release Note Template for Downstream PRs (will be copied)

dialogflowcx: added `is_default_welcome_intent` and `is_default_negative_intent` fields to `google_dialogflow_cx_intent` resource to allow management of default intent resources via Terraform

dialogflowcx: added `is_default_start_flow` field to `google_dialogflow_cx_flow` resource to allow management of default flow resources via Terraform

[modular-magician]

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

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

[mmurakowski-verily]

FYI, setting the is_fallback field has never worked. If you try creating a resource with it set, you get Error creating Intent: googleapi: Error 400: com.google.apps.framework.request.BadRequestException: You cannot create fallback intent; if you try updating it on an existing resource you get Error updating Intent "[REDACTED]": googleapi: Error 400: com.google.apps.framework.request.BadRequestException: Regular intent can not be converted to a fallback intent.

So there's no users that should be broken here.

UPDATE: ugh, I thought of a case where this might break existing users: if they'd imported a Default Negative Intent. Undoing the change :( :( :(

[SarahFrench]

👋 I just did an initial pass after reading the issue - I need to have another proper look at the tests but I identified a required schema change that should be addressed first.

Also, I added a release note about the field being changed from an argument to an output/attribute. From what you've described making that change in this PR isn't a problem despite it being a breaking change

[mmurakowski-verily]

Also, I added a release note about the field being changed from an argument to an output/attribute.

Good catch, thanks!

[melinath]

@SarahFrench @rileykarson I think it might be possible to handle this case by making the name field optional + computed instead of adding these new fields / complex behavior? Similar to

magic-modules/mmv1/products/workflows/Workflow.yaml

Line 63 in c7d666d

- !ruby/object:Api::Type::String

[rileykarson]

That resource is O+C to support a name_prefix field, this one wants acquire-on-create behaviour for just the default flow / intent. At a surface level it is possible to make the field specifiable, sure, but that's just the first step in making the resource acquire-on-create.

The Azure provider experimented with more general acquire-on-create because most Azure APIs are Upsert APIs, and actively took steps to walk that back: https://github.com/hashicorp/terraform-provider-azurerm/blob/497f844925d50321ee1eea10bb1d7c13662828ea/website/docs/guides/2.0-upgrade-guide.html.markdown#changes-when-importing-existing-resources

We want to avoid that because it makes Terraform much more likely to touch unintended settings- acquire-on-create does not message what it is doing in plan.

[rileykarson]

If we have a different resource doing something similar as precedent, we should consider that (and I highlighted one potential one in chat for Sarah to consider referencing, logging bucket config)- but please don't use https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/workflows_workflow as a precedent for this change as it's completely different.

[mmurakowski-verily]

PS to be clear, I don't have a horse in this race, I just want one no-argument terraform apply to do everything. I can make a third PR with google_dialogflow_cx_default_start_flow, google_dialogflow_cx_default_welcome_intent, and google_dialogflow_cx_default_negative_intent resources, and let y'all professional terraformers decide which one you like best.

[melinath]

@rileykarson I'm not saying that google_workflows_workflow is a precedent for acquire-on-create. My understanding from talking to @mmurakowski-verily was that creating this resource with no name set ends up using the default name - so using O+C would be an acceptable way to represent that, similar to google_workflows_workflow (and other resources) that have a name that is either set by the server or the user.

It sounds like there may have been a misunderstanding if that's not accurate.

[rileykarson]

What's different here is that there's a single singleton per-parent, and all other resources of this kind can be managed entirely through Terraform. For those Terraform-managed ones the name is server-defined (and the singletons have a fixed name).

In Workflows the name is client-defined and we're computing a name entirely locally- see https://cloud.google.com/workflows/docs/reference/rest/v1/projects.locations.workflows/create#query-parameters, the name is a required field. We're just setting it in the encoder when unset in config.

There are similarities at a surface level (both can have a name determined at apply-time) but they're not cases of the same problem.

Some much closer cases are https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/logging_project_bucket_config and https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/firestore_database where there's a singleton (or two, in logging's case) and other instances can be entirely Terraform-managed.

[modular-magician]

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 ( 8 files changed, 661 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 9 files changed, 662 insertions(+), 1 deletion(-))
TF OiCS: Diff ( 16 files changed, 583 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3178
Passed tests 2855
Skipped tests: 319
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

TestAccDataprocClusterIamPolicy|TestAccDialogflowCXIntent_dialogflowcxIntentDefaultNegativeIntentExample|TestAccDialogflowCXIntent_defaultIntents|TestAccHealthcareDatasetIamPolicy

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDialogflowCXIntent_dialogflowcxIntentDefaultNegativeIntentExample[Debug log]
TestAccDialogflowCXIntent_defaultIntents[Debug log]
TestAccHealthcareDatasetIamPolicy[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

[melinath]

yeah, I misunderstood the ask from @mmurakowski-verily and thought this was much simpler than it is. My bad & thanks for the additional context.

It seems like the implementation proposed here would allow users to more smoothly set up an agent and the default flow without needing to have an additional step to import it - but it doesn't allow the user to set up custom flows / intents because the name field is not settable (NVM, currently supports custom flows and just sets a random ID at create time). Meanwhile the other implementation allows custom flows / intents doesn't have new fields but relies on magic name fields to change internal behavior, which feels a little scary.

Possibly this API didn't initially support custom flows / intents originally?

At the risk of proposing another option that makes no sense: It feels like the most intuitive way to handle this in Terraform would be to assume that users generally want to create new flows, not use the default flow. I'm not quite clear on whether the default flows have a "special status" - for example, google_logging_project_bucket_config configures a singleton that applies to a broad set of resources - you can't just create a new one and use it instead.~

It seems like flows / intents aren't really singletons, just easily available defaults. @mmurakowski-verily can you confirm? How necessary is it to use the default flows / intents?

If so - we could potentially switch to an Optional + Computed field for name (should be backwards-compatible since the field is already in state?) and for new resources folks could have a choice between importing the default resource or setting an explicit id?

[melinath]

Hm, taking another look - using O+C would only make sense if the API allows users specifying an ID at create time - so that would only make sense if the create method supports the name being passed in the body (which would be a little unusual - standard behavior is to pass it in the URL)

[melinath]

I'm not going to leave any more comments on here to avoid muddying the waters here more 😂 Sarah is the reviewer.

[mmurakowski-verily]

@SarahFrench I've re-opened this + #9359; pick the one you like, or tell me to write a new one for google_dialogflow_cx_default_start_flow etc resources if that's your preferred fix for hashicorp/terraform-provider-google#16308.

Thanks in advance for doing the hard part of this process.

[SarahFrench]

Sorry for the wait - here's a review that's mostly me asking for small updates to help users and maintainers notice that the new virtual fields don't follow assumptions they may have about how resources behave.

I did a bunch of manual testing of the code changes and everything worked as expected 🎉 I may do one more review but this PR feels mostly finished already 😁

[modular-magician]

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 ( 8 files changed, 663 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 9 files changed, 664 insertions(+), 1 deletion(-))
TF OiCS: Diff ( 16 files changed, 583 insertions(+))

[modular-magician]

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 ( 8 files changed, 675 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 9 files changed, 676 insertions(+), 1 deletion(-))
TF OiCS: Diff ( 16 files changed, 583 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3202
Passed tests 2877
Skipped tests: 325
Affected tests: 0

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

[modular-magician]

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 ( 8 files changed, 842 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 9 files changed, 843 insertions(+), 1 deletion(-))
TF OiCS: Diff ( 16 files changed, 583 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3203
Passed tests 2876
Skipped tests: 325
Affected tests: 2

Action taken

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

TestAccDialogflowCXFlow_defaultStartFlow|TestAccDialogflowCXIntent_defaultIntents

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDialogflowCXFlow_defaultStartFlow[Debug log]
TestAccDialogflowCXIntent_defaultIntents[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

[SarahFrench]

👋 @mmurakowski-verily I'm happy to go ahead and merge; the changes to the resources behave as expected and we've included a bunch of notices to maintainers and users about the atypical behaviour of the resource when these virtual fields are used.

Thank you for this contribution, and also for expanding what we can do with virtual fields!