Handle additionalProperties #963

praneetloke · 2024-02-14T04:30:10Z

This should fix #401.

As stated in the linked issue, the type for the stateful policy of a managed instance group in the compute module should be a mapping of string and an object that accepts the value for the auto-delete rule (enum). The possible values for the auto-delete rule are NEVER and ON_PERMANENT_INSTANCE_DELETION.

I looked into this briefly and found that the discovery doc for compute.v1 has the right mapping but my guess is that when generating the schema, the generator is dropping the $ref on the additionalProperties which turns these into a simple mapping instead of a map of maps.

Here's a snippet from the discovery doc on the master branch:

"StatefulPolicyPreservedState": {
      "description": "Configuration of preserved resources.",
      "id": "StatefulPolicyPreservedState",
      "properties": {
        "disks": {
          "additionalProperties": {
            "$ref": "StatefulPolicyPreservedStateDiskDevice"
          },
          "description": "Disks created on the instances that will be preserved on instance delete, update, etc. This map is keyed with the device names of the disks.",
          "type": "object"
        },
        "externalIPs": {
          "additionalProperties": {
            "$ref": "StatefulPolicyPreservedStateNetworkIp"
          },
          "description": "External network IPs assigned to the instances that will be preserved on instance delete, update, etc. This map is keyed with the network interface name.",
          "type": "object"
        },
        "internalIPs": {
          "additionalProperties": {
            "$ref": "StatefulPolicyPreservedStateNetworkIp"
          },
          "description": "Internal network IPs assigned to the instances that will be preserved on instance delete, update, etc. This map is keyed with the network interface name.",
          "type": "object"
        }
      },
      "type": "object"
    },

And here's the Pulumi schema for v1:

"google-native:compute/v1:StatefulPolicy": {
            "properties": {
                "preservedState": {
                    "type": "object",
                    "$ref": "#/types/google-native:compute/v1:StatefulPolicyPreservedState"
                }
            },
            "type": "object"
        },
        "google-native:compute/v1:StatefulPolicyPreservedState": {
            "description": "Configuration of preserved resources.",
            "properties": {
                "disks": {
                    "type": "object",
                    "description": "Disks created on the instances that will be preserved on instance delete, update, etc. This map is keyed with the device names of the disks."
                },
                "externalIPs": {
                    "type": "object",
                    "description": "External network IPs assigned to the instances that will be preserved on instance delete, update, etc. This map is keyed with the network interface name."
                },
                "internalIPs": {
                    "type": "object",
                    "description": "Internal network IPs assigned to the instances that will be preserved on instance delete, update, etc. This map is keyed with the network interface name."
                }
            },
            "type": "object"
        },

Note how all properties of StatefulPolicyPreservedState are missing the additionalProperties definition. (I guess the code generator is defaulting properties of type: object without any other attributes as maps?)

This PR handles additionalProperties for both cases where a property is a map with a primitive value or one where the value is a reference to a type.

github-actions · 2024-02-14T04:30:43Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

praneetloke · 2024-02-14T16:37:10Z

provider/pkg/gen/schema.go

+			return &schema.TypeSpec{
+				Type: "object",
+				AdditionalProperties: &schema.TypeSpec{
+					Type: "string",


This should not be hard-coded to string. Ideally, this uses prop.AdditionalProperties.Type but that leads to some errors that I didn't look deeply into. But then again, perhaps a string is ok here for now?

it's true that these are usually strings, but I think if you instead recurse on prop.AditionalProperties, you can get the full type.

praneetloke · 2024-02-26T18:05:01Z

Ping.

mjeffryes

First off: thanks for contributing this PR! And apologies that it's taken us so long to review.

Conceptually, I think this is about right, but there are a couple nuances I'd like to make sure we get correct. And you'll also need to run a full make build to regenerate all the sdks before we can run tests.

FWIW, we recently made a similar change in the generator for the aws-native cloud control provider; you might look at this part for some inspiration/comparison: https://github.com/pulumi/pulumi-aws-native/pull/1342/files#diff-a462bb9d1699adb657844ae5c68deb17b7edfd49a2a39ab28f7f53e7e8a380b7

mjeffryes · 2024-03-02T00:48:37Z

provider/pkg/gen/schema.go

+			return &schema.TypeSpec{
+				Type: "object",
+				AdditionalProperties: &schema.TypeSpec{
+					Type: "string",


it's true that these are usually strings, but I think if you instead recurse on prop.AditionalProperties, you can get the full type.

mjeffryes · 2024-03-02T00:49:27Z

provider/pkg/gen/schema.go

+
+		// Otherwise, the value in-turn is a complex type.
+		typePropName := fmt.Sprintf(`%s%s`, typeName, strings.Title(propName))
+		return g.genTypeSpec(typePropName, propName, prop.AdditionalProperties, isOutput)


I think this ends up just return the type of the value rather than a map from string -> valueType

mikhailshilkov · 2024-04-09T07:36:31Z

@praneetloke Are you still planning to address Matt's suggestions here?

praneetloke · 2024-04-09T15:18:45Z

@mikhailshilkov I am going to try to get to this this week. If not, I'll close it. I really think this should be finished though, to improve the quality of this native provider. As it stands right now, the bug this is trying to fix corrects a lot of inaccuracies in the schema as you can see in the diff.

github-actions · 2024-04-27T21:54:32Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-27T21:55:42Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-28T01:42:19Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-28T01:49:29Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

praneetloke · 2024-04-28T01:50:45Z

provider/pkg/gen/overrides.go

+		// AdditionalProperties type spec.
+		//
+		// Discovery doc: https://servicemanagement.googleapis.com/$discovery/rest?version=v1
+		"overridesByRequestProtocol": {


Hopefully, the above comment is self-explanatory. If you have a different approach to fixing this or think that the recursive type is actually valid in this case, please let me know and I can revert this. However, that would mean that a solution for the recursive reference would be needed for this provider's schema (perhaps similar to k8s' ObjectMeta?)

I discovered this issue when I ran make build. Here's how the property is defined in the discovery doc:

"BackendRule": { ... ... "overridesByRequestProtocol": { "additionalProperties": { "$ref": "BackendRule" }, "description": "The map between request protocol and the backend address.", "type": "object" }, ... ... }

My reading of that schema is that it's probably supposed to be recursive-ish. The field name suggests that you could override any of the values in BackendRule for a particular protocol. It's probably nonsensical to have an "overridesByRequestProtocol" within an "overridesByRequestProtocol" parameter. However, I suspect a string map won't work, because it's probably expecting a map of objects object instead of a map of strings.

The safest thing to do would probably be to omit this property entirely until someone can take the time to explore the behavior of this api

My reading of that schema is that it's probably supposed to be recursive-ish. The field name suggests that you could override any of the values in BackendRule for a particular protocol.

It's even more strange than that. From looking at the APIs you can't create a backend rule. This is just used in the response to retrieve (get) a service config. So I don't know how this is used and the documentation for the field is conflicting with how the type is represented. Having said that, I kind of agree with you that this is probably supposed to be recursive without the overridesByRequestProtocol being present in the recursed types.

I can exclude the property entirely for now instead of doing this.

The safest thing to do would probably be to omit this property entirely until someone can take the time to explore the behavior of this api

This is done.

praneetloke · 2024-04-28T01:56:52Z

@mjeffryes @mikhailshilkov I've updated this PR. I've separated the SDK regen as a separate commit[1]. I think I have addressed the comments. I also uncovered a problem in the discovery doc for servicemanagement. Please have a look at that.

[1]=It's a large number of changes. But let's be honest, this is not the largest number of changes I've made in a PR 😄

github-actions · 2024-04-28T17:09:36Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-05-03T18:22:07Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

mjeffryes

Left a few comments on some edge cases, but I think this is pretty close. I spot checked some of the schema changes and it definitely looks like an improvement. We'll also need some tests for the new behavior to land this.

mjeffryes · 2024-05-03T17:42:33Z

provider/pkg/gen/overrides.go

+		// AdditionalProperties type spec.
+		//
+		// Discovery doc: https://servicemanagement.googleapis.com/$discovery/rest?version=v1
+		"overridesByRequestProtocol": {


My reading of that schema is that it's probably supposed to be recursive-ish. The field name suggests that you could override any of the values in BackendRule for a particular protocol. It's probably nonsensical to have an "overridesByRequestProtocol" within an "overridesByRequestProtocol" parameter. However, I suspect a string map won't work, because it's probably expecting a map of objects object instead of a map of strings.

The safest thing to do would probably be to omit this property entirely until someone can take the time to explore the behavior of this api

mjeffryes · 2024-05-03T17:53:36Z

provider/pkg/gen/schema.go

+			default:
+				return &schema.TypeSpec{
+					Type: "object",
+					AdditionalProperties: &schema.TypeSpec{
+						Type: prop.AdditionalProperties.Type,
+					},
+				}, nil
+			}


This is fine for "boolean", "number", "integer", & "string"; however, we should exclude "null" and "object" types from this. ("null" is not a valid pulumi type and "object" does not make sense without any parameters)

mjeffryes · 2024-05-03T17:58:06Z

provider/pkg/gen/schema.go

+				// An array type for AdditionalProperties doesn't make sense
+				// since it's already a map and multiple entries are
+				// allowed by definition. So generate a type
+				// based on the type of the "items" in the array.


I disagree, a map[string][]T is a perfectly valid type (eg. you might have a map from team name to employees list).

a map[string][]T is a perfectly valid type (eg. you might have a map from team name to employees list).

Yes. But I wasn't referring to that :) My confusion was about type: array and what that is referring to. I guess I didn't realize that the type: array here is indicating a list of values for each key. If that is the case, then yeah what I have here is definitely wrong. I'll change this.

I've fixed this now.

By the way, the reason why the array type didn't make sense to me in the first place in this context is, if you look at the UI for creating a budget, you can only have a single value for a key:

Having said that I agree that this can't be generalized like how I did at first because there could be a <string, any[]> elsewhere in the future that this would break.

mjeffryes · 2024-05-03T18:26:08Z

/run-acceptance-tests

pulumi-bot · 2024-05-03T18:26:48Z

Please view the PR build: https://github.com/pulumi/pulumi-google-native/actions/runs/8943299715

github-actions · 2024-05-03T19:43:07Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

mjeffryes · 2024-05-14T23:38:40Z

Thanks for updating this @praneetloke; I think we still need to address #963 (comment) and add some test cases to wrap this up. If I get a chance I will try to see if I can add these, but it may be a while so you might beat me to it.

mjeffryes · 2024-06-27T00:43:13Z

/run-acceptance-tests

github-actions · 2024-06-27T00:43:35Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

pulumi-bot · 2024-06-27T00:43:57Z

Please view the PR build: https://github.com/pulumi/pulumi-google-native/actions/runs/9688797682

…nd regen schema

mjeffryes · 2024-07-01T14:38:52Z

/run-acceptance-tests

github-actions · 2024-07-01T14:42:53Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

pulumi-bot · 2024-07-01T14:44:14Z

Please view the PR build: https://github.com/pulumi/pulumi-google-native/actions/runs/9746313392

mjeffryes · 2024-07-02T15:15:07Z

/run-acceptance-tests

github-actions · 2024-07-02T15:15:18Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

pulumi-bot · 2024-07-02T15:15:52Z

Please view the PR build: https://github.com/pulumi/pulumi-google-native/actions/runs/9763625935

mjeffryes · 2024-07-02T15:53:28Z

Builds passed (finally)!

praneetloke commented Feb 14, 2024

View reviewed changes

mjeffryes reviewed Mar 2, 2024

View reviewed changes

praneetloke commented Apr 28, 2024

View reviewed changes

praneetloke requested a review from mjeffryes April 28, 2024 01:54

mjeffryes reviewed May 3, 2024

View reviewed changes

praneetloke requested a review from mjeffryes May 3, 2024 19:43

praneetloke added 6 commits July 1, 2024 07:22

Handle additionalProperties

d6b6270

Don't hardcode the type

f1c9f64

Regen schema

2ca6307

Add go.work file

0e413da

Fix condition

42d40df

Fix incorrect recursive property ref for overridesByRequestProtocol a…

51988b2

…nd regen schema

praneetloke and others added 8 commits July 1, 2024 07:30

Rebuild the SDKs

8a17e5b

Add a note about the overrides

418841f

Refs should be maps

03557c4

Regen SDKs again

5fc7258

bless for testing

6b15447

Address PR feedback

5ad4777

Regen SDKs

9dba312

error cases and test

023d86c

rm go.work (trips up CI)

e6f2aed

mjeffryes approved these changes Jul 2, 2024

View reviewed changes

mjeffryes merged commit 551f977 into pulumi:master Jul 2, 2024
6 of 7 checks passed

Handle additionalProperties #963

Handle additionalProperties #963

Conversation

praneetloke commented Feb 14, 2024 • edited Loading

github-actions bot commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praneetloke commented Feb 26, 2024

mjeffryes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov commented Apr 9, 2024

praneetloke commented Apr 9, 2024

github-actions bot commented Apr 27, 2024

github-actions bot commented Apr 27, 2024

github-actions bot commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praneetloke commented Apr 28, 2024

github-actions bot commented Apr 28, 2024

github-actions bot commented May 3, 2024

mjeffryes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjeffryes commented May 3, 2024

pulumi-bot commented May 3, 2024

github-actions bot commented May 3, 2024

mjeffryes commented May 14, 2024 • edited Loading

mjeffryes commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

pulumi-bot commented Jun 27, 2024

mjeffryes commented Jul 1, 2024

github-actions bot commented Jul 1, 2024

pulumi-bot commented Jul 1, 2024

mjeffryes commented Jul 2, 2024

github-actions bot commented Jul 2, 2024

pulumi-bot commented Jul 2, 2024

mjeffryes commented Jul 2, 2024

praneetloke commented Feb 14, 2024 •

edited

Loading

mjeffryes commented May 14, 2024 •

edited

Loading