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

google_cloud_run_service: status field should have MaxItems set to 1 #9771

Closed
cakoose opened this issue Aug 11, 2021 · 10 comments
Closed

google_cloud_run_service: status field should have MaxItems set to 1 #9771

cakoose opened this issue Aug 11, 2021 · 10 comments
Assignees
Labels
bug forward/review In review; remove label to forward service/run

Comments

@cakoose
Copy link

cakoose commented Aug 11, 2021

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request.
  • Please do not leave +1 or me too comments, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.
  • If an issue is assigned to the modular-magician user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If an issue is assigned to a user, that user is claiming responsibility for the issue. If an issue is assigned to hashibot, a community member has claimed the issue already.

Terraform Version

N/A. I'm using Pulumi, which uses the Terraform source code. The bug I'm reporting is based on the Terraform source code.

Affected Resource(s)

Terraform Configuration Files

N/A

Debug Output

N/A

Panic Output

N/A

Expected Behavior

The status field should have MaxItems set to 1. For example, the metadata and spec fields have MaxItems to 1.

Actual Behavior

The MaxItems attribute is not set: https://github.com/hashicorp/terraform-provider-google-beta/blob/11c674f5f4edf7e87c2d1c94cfb0d69cde7b9af4/google-beta/resource_cloud_run_service.go#L680

Steps to Reproduce

N/A

Important Factoids

N/A

References

The GCP API definition of the Cloud Run Service object: link.

  • metadata, spec, and status are all non-repeating fields. However in Terraform, only metadata and spec have MaxItems set to 1.

My original bug report against the Pulumi project: pulumi/pulumi#7634

A tangentially-related bug report about the status field: #8728

@cakoose cakoose added the bug label Aug 11, 2021
@edwardmedia edwardmedia self-assigned this Aug 11, 2021
@edwardmedia
Copy link
Contributor

@cakoose below is the api that defines what status contains. I don't see MaxItems available in the block. Can you provide the Terraform execution debug log or the source that leads you to think it is the case?

https://cloud.google.com/run/docs/reference/rest/v1/namespaces.services#ServiceStatus

@cakoose
Copy link
Author

cakoose commented Aug 12, 2021

Oh, sorry, I wasn't clear. Here's the full reasoning:

The GCP Cloud Run Service object has a field status. The type of the field is a single object with field like currentGeneration, conditions, etc. (matching the definition you linked to).

In Terraform, the Service object's status field is defined as a list of those objects (link) instead of just a single one.

My understanding of the Terraform schema language is a few years old, but:

  • I believe status is defined as a Type: schema.TypeList because that's the only way in Terraform to have nested fields.
  • For nested fields that aren't actually a list, I think a common Terraform pattern is to set MaxItems: 1 in the list's schema definition. For example, that's what's done for the metadata field: link.

All I'm saying is that I think the status field's Terraform schema definition should also have MaxItems: 1 set.

@edwardmedia
Copy link
Contributor

@cakoose I know what you mean. Status is defined as part of the output. Does it make difference whether we limit it MaxItems:1?

@cakoose
Copy link
Author

cakoose commented Aug 12, 2021

Ah I think I see what you're saying:

  • For inputs, setting MaxItems: 1 prevents users from accidentally writing configs with multiple elements.
  • For outputs, it's less useful because that data comes from the remote API and is likely to be correct.

The reason it would still be useful for to set MaxItems: 1 on the output: We use a tool that does support nested objects natively (as opposed to hacking it with a single-item list). That tool uses the Terraform schema's MaxItems and MinItems properties to determine whether a Terraform schema.TypeList is actually just a nested object. Making the Terraform schema more precise would help that tool.

@rileykarson
Copy link
Collaborator

@cakoose
Copy link
Author

cakoose commented Aug 12, 2021

Oh, looks like you are right: https://www.terraform.io/docs/extend/guides/v2-upgrade-guide.html#stronger-validation-for-helper-schema-schema-computed-fields

That's unfortunate, but changing that decision is probably way too hard at this point :-\ Either way, thank you so much for getting to the bottom of it!

Tangent: Do you have an opinion on the lack of direct support for nested fields? I was surprised when I found this out ~5 years ago and am surprised that it still hasn't been added. But I assume you know Terraform much better than I do -- is there a reason for this?

@rileykarson
Copy link
Collaborator

I think the model of Terraform Core assumed that fields were either a primitive or had multiple elements, and didn't account for a nested object type early on, and that got carried forward. There used to be a typeObject stub beside TypeString, TypeMap, TypeList etc, not sure if that's still around.

As of 0.12 the core restriction is gone, so the future SDK should include support.

Also, closing as infeasible.

@cakoose
Copy link
Author

cakoose commented Aug 12, 2021

As of 0.12 the core restriction is gone, so the future SDK should include support.

Not sure I follow -- does this mean that newer SDKs will be based on a different schema language? Or will the existing schema language be enhanced to support a nested object? Is there somewhere I can read more about the progress here?

@rileykarson
Copy link
Collaborator

New schema language if that means what I think, yep. https://www.terraform.io/docs/plugin/framework/index.html, https://github.com/hashicorp/terraform-plugin-go are good references.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2021
@github-actions github-actions bot added service/run forward/review In review; remove label to forward labels Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug forward/review In review; remove label to forward service/run
Projects
None yet
Development

No branches or pull requests

3 participants