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

Add case of valueUnknown in configId pre-computing #8853

Conversation

oshiro3
Copy link
Contributor

@oshiro3 oshiro3 commented Sep 4, 2023

When certain attributes such as protoc_output_base64 are changed, config_id is pre-compute, 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 and Error: Provider produced inconsistent final plan.
Therefore, if those attributes have been changed and their values are non-deterministic, config_id is also calculated at apply time.

I was going to add a test, but I couldn't figure out how to do it without using an external provider such as null_resource for certain attributes, so I didn't implement it.
If there is a better way, please let me know.

issue: hashicorp/terraform-provider-google#11776

Release Note Template for Downstream PRs (will be copied)

ResourcemangemntEndpointsService: Modify precomputed config_id

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

@roaks3, 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 Sep 4, 2023
@roaks3
Copy link
Contributor

roaks3 commented Sep 6, 2023

@oshiro3 the logic here makes sense to me, but I'd like to confirm that this is in fact the behavior of the API for all 3 of these fields (and it results in an error). One thing that might help is tests, and I'm not clear on where you found trouble. Is it possible to recreate the config that failed in the issue, and make sure it succeeds?

@oshiro3
Copy link
Contributor Author

oshiro3 commented Sep 7, 2023

@roaks3
Thanks for confirming.
I'm really sorry I didn't write down the steps to reproduce the problem.
This problem was occurring when using the following code
And this was difficult to raise as working code in the mm repository because it requires a hashicorp module.
In the case of gRPC, it is not uncommon to obtain api_config and api_descriptor from external sources.
However, in the case of getting api_config and api_descriptor as external files, it is possible for them to become known_after_apply for some reason, even though they are null_resource in this case.

resource "null_resource" "test1" {
  triggers = {
    always = timestamp()
  }

  provisioner "local-exec" {
    command = "echo ok"
  }
}

data "local_file" "test_api_desc" {
  filename = "./test_api_descriptor.pb"
  depends_on = [
    resource.null_resource.test1
  ]
}

resource "google_endpoints_service" "endpoints_service" {
  service_name = "service_name"
  project      = "project"
  grpc_config  = <<EOF
type: google.api.Service
config_version: 3
name: tf-test.endpoints.project.cloud.goog
usage:
  rules:
  - selector: endpoints.examples.bookstore.Bookstore.ListShelves
    allow_unregistered_calls: true
EOF

  protoc_output_base64 = chomp(data.local_file.test_api_desc.content_base64)
}

@roaks3
Copy link
Contributor

roaks3 commented Sep 7, 2023

Sorry, I'm not exactly following. Could we use your example (ie. testAccEndpointsService_grpc), and then update grpc_config with a value that would be (known after apply)? You might be able to trigger that by including a value that references an id of a resource that isn't yet created, or something similar (I assume this is the purpose of the null resources you're using?).

@oshiro3
Copy link
Contributor Author

oshiro3 commented Sep 17, 2023

Sorry for the delay.
I don't think I have an accurate picture of what your concerns are.
I am not sure if your concern or what you want to see is with the Issue itself, or with the changes I have made to the P/R, or with the resulting behavior.

Could we use your example (ie. testAccEndpointsService_grpc), and then update grpc_config with a value that would be (known after apply)?

Test may be incomplete in this regard, but I think the code I put in one previous comment at least does the behavior I expect.

@roaks3
Copy link
Contributor

roaks3 commented Sep 25, 2023

Basically I think we need to include a test for this behavior, and I believe that this could be accomplished without a null_resource.

I would try something like this if you are stuck on that:

...
resource "google_tags_tag_key" "key" {
  parent = "organizations/123456789"
  short_name = "endpoints-service-test"
}

resource "google_endpoints_service" "endpoints_service" {
  service_name = "service_name"
  project      = "project"
  grpc_config  = <<EOF
type: google.api.Service
config_version: 3
name: tf-test.endpoints.project.cloud.goog
title: Test ${google_tags_tag_key.key.name}
usage:
  rules:
  - selector: endpoints.examples.bookstore.Bookstore.ListShelves
    allow_unregistered_calls: true
EOF

  protoc_output_base64 = chomp(data.local_file.test_api_desc.content_base64)
}

@oshiro3
Copy link
Contributor Author

oshiro3 commented Nov 1, 2023

I have created a test based on your comment, I am not sure I understand the behavior of the tag_key resource well, but does it become a test that fully meets the specification?

@oshiro3
Copy link
Contributor Author

oshiro3 commented Nov 27, 2023

@roaks3
Sorry for the delay, but I need you to take re-review, is it ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants