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

Fixed a problem in which only ConfigId was calculated first when the API parameter was set to Known After Apply #9708

Conversation

oshiro3
Copy link
Contributor

@oshiro3 oshiro3 commented Dec 22, 2023

If certain attributes such as protoc_output_base64 are modified, the config_id is pre-computed, but if these attributes are (known after apply) at plan time and there is no diff as a result of apply, the plan result will not match, Error: Provider produced inconsistent final plan.
Therefore, if these attributes are modified and their values are non-deterministic, the config_id is now also calculated at apply time.

The added test case has the dependency endpoints <- key2 <- key1, and the attribute of key2 is used in grpc_config of endpoints.
If the attribute of key2 referenced in endpoints is known after apply for some reason, and as a result there is no diff in the attribute of key2, only the config_id will be changed, but in that case it will not be applied and Provider produced inconsistent final plan error happened.

As a specific case, we have confirmed the occurrence of this problem in the product code in cases where grpc-config, protoc-descriptor, etc. are obtained through multiple resources such as local_file, etc. and external files obtained by null_resource.

I have closed P/R and newly opened because discovering that the previous P/R had many incorrect modified source code, corrections, tests, etc.

Fixes hashicorp/terraform-provider-google#11776

Release Note Template for Downstream PRs (will be copied)

servicemanagement: fixed issue in `google_endpoints_service` where an inconsistent plan would be created when certain fields had computed values 

@modular-magician
Copy link
Collaborator

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

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

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 22, 2023
@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jan 3, 2024
@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 ( 2 files changed, 63 insertions(+))
Terraform Beta: Diff ( 2 files changed, 63 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 9
Passed tests 7
Skipped tests: 1
Affected tests: 1

Click here to see the affected service packages
  • servicemanagement

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
TestAccEndpointsService_grpcNotPreComputeConfigId

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccEndpointsService_grpcNotPreComputeConfigId[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

@oshiro3
Copy link
Contributor Author

oshiro3 commented Jan 8, 2024

@NickElliot
I apologize for the inconvenience, but have you had a chance to review this P/R?
If you have any problems or questions, or if you think this P/R should be closed, I would greatly appreciate your comments to that effect.

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

Mostly LGTM! The logic makes sense and the new test fulfills the conditions of the computed fields no longer producing errors. Would it be possible to add another test that verifies for openapi_config in addition to the other two currently covered fields?

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jan 9, 2024
@oshiro3
Copy link
Contributor Author

oshiro3 commented Jan 9, 2024

@NickElliot
Thanks for the appropriate review.
I was able to extend the test for the open_api_config and grpc_config parameters, but could not come up with a good way to make protoc_output_base64 known_after_apply without changing the content.
I believe this can be reproduced during testing by using local_file, null_resource, etc. or by creating a temporary file, but if you have a better way, please let me know.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jan 9, 2024
@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 ( 2 files changed, 150 insertions(+))
Terraform Beta: Diff ( 2 files changed, 150 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 10
Passed tests 7
Skipped tests: 1
Affected tests: 2

Click here to see the affected service packages
  • servicemanagement

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
TestAccEndpointsService_grpcNotPreComputeConfigIdByGrpcConfig|TestAccEndpointsService_openapiNotPreComputeConfigId

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

This should be fine as is. Thanks for the contribution!

@NickElliot NickElliot merged commit 37b21aa into GoogleCloudPlatform:main Jan 9, 2024
12 checks passed
bskaplan pushed a commit to bskaplan/magic-modules that referenced this pull request Jan 17, 2024
…elds were unknown at plan time (GoogleCloudPlatform#9708)

* Add case of valueUnknown in configId pre-computing

* add openapi parameters test
kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
…elds were unknown at plan time (GoogleCloudPlatform#9708)

* Add case of valueUnknown in configId pre-computing

* add openapi parameters test
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…elds were unknown at plan time (GoogleCloudPlatform#9708)

* Add case of valueUnknown in configId pre-computing

* add openapi parameters test
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…elds were unknown at plan time (GoogleCloudPlatform#9708)

* Add case of valueUnknown in configId pre-computing

* add openapi parameters test
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.

google_endpoints_service detects false changes when passing openapi config as template_file
3 participants