-
Notifications
You must be signed in to change notification settings - Fork 2k
Add fine-grained resource GenerativeSettings to DialogflowCX #14169
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
Add fine-grained resource GenerativeSettings to DialogflowCX #14169
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 32 Click here to see the affected service packages
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
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 32 Click here to see the affected service packages
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
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
We need to set fallback settings because two prompt_templates are automatically created. Otherwise, update test would fail with an `After applying this test step, the plan was not empty` error.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 32 Click here to see the affected service packages
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
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @NickElliot, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between language_code
and lang_code
...hird_party/terraform/services/dialogflowcx/resource_dialogflowcx_generative_settings_test.go
Outdated
Show resolved
Hide resolved
|
…esource_dialogflowcx_generative_settings_test.go Co-authored-by: Nick Elliot <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 36 Click here to see the affected service packages
View the build log |
Co-authored-by: Nick Elliot <[email protected]>
Co-authored-by: Nick Elliot <[email protected]>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 36 Click here to see the affected service packages
View the build log |
Tests analyticsTotal tests: 36 Click here to see the affected service packages
View the build log |
@NickElliot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are langCode and languageCode always the same value? my question is more or less wondering if the "url_param_only" langCode could be replaced by having languageCode as a field be required and also the URL parameter
@NickElliot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
langCode and languageCode won't be always the same value. Users can specify an invalid langCode in the Get request, in which case they will receive an error response.
is a langCode invalid if it doesnt match the languageCode? also is languageCode an exported attribute i.e. will the API return a value for the getGenerativeSettings with the field even if a user does not explicitly supply that?
it doesnt necessarily matter that get request doesnt supply the generative settings object, this would just be simplifying the Terraform config. Is there an actual scenario where a user will have a different value for both of these fields that doesn't produce an error?
Yes. A dialogflow agent can support multiple languages, and each of them is associated with a
No. Example https://screenshot.googleplex.com/AkF6L5xv2LtdgDT
As long as langCode is one of the supported languages of the agent, the get request won't produce an error. For example, an agent can support two languages |
@NickElliot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on what you said and reviewing the documentation, it seems like langCode
is the same value as languageCode
within a given instance of the fine grained resource, being functionally the Unique Identifier for the fine grained resource.
Could you remove the langCode
url parameter and update the self_link
of the resource to be ?languageCode={{language_code}}', and set
languageCode` as required?
langCode is the same value as languageCode within a given instance of the fine grained resource, being functionally the Unique Identifier for the fine grained resource.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 36 Click here to see the affected service packages
View the build log |
Thanks for the suggestion! I've dropped the the |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 36 Click here to see the affected service packages
View the build log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
3ae4e9c
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
…loudPlatform#14169) Co-authored-by: Nick Elliot <[email protected]>
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.