pulumi / pulumi-google-native

Apache License 2.0
71 stars 18 forks source link

Property patterns are only in descriptions #132

Open mikhailshilkov opened 3 years ago

mikhailshilkov commented 3 years ago

Let's take two examples:

  1. A compute instance requires a machineType property that is defined as
"machineType": {
  "description": "Full or partial URL of the machine type resource to use for this instance, in the format: zones/zone/machineTypes/machine-type. This is provided by the client when the instance is created. For example, the following is a valid partial url to a predefined machine type:\nzones/us-central1-f/machineTypes/n1-standard-1\n\n\nTo create a custom machine type, provide a URL to a machine type in the following format, where CPUS is 1 or an even number up to 32 (2, 4, 6, ... 24, etc), and MEMORY is the total memory for this instance. Memory must be a multiple of 256 MB and must be supplied in MB (e.g. 5 GB of memory is 5120 MB):\nzones/zone/machineTypes/custom-CPUS-MEMORY\n\n\nFor example: zones/us-central1-f/machineTypes/custom-4-5120 \n\nFor a full list of restrictions, read the Specifications for custom machine types.",
  "type": "string"
},
  1. A cloud function accepts and returns a name property defined as
"name": {
  "description": "A user-defined name of the function. Function names must be unique globally and match pattern `projects/*/locations/*/functions/*`",
  "type": "string"
}

Both expect a value of a predefined structure, which is hard for end users to figure out.

We would love to build/parse this structure automatically from raw values but it's hard to automate this without extra metadata in addition to the textual description.

codyoss commented 3 years ago

For name fields I think you might be able to get this information is a parsable format by cross-referencing a proto annotation on the GET operation of the resource. Example from Cloud Functions protos.

mikhailshilkov commented 3 years ago

@codyoss The URL params are more-or-less under control. The question is whether those patterns can be applied to properties in message bodies. For your example above, this name property is a part of a protobuf message and has no format annotation. It has the same format - but would that hold universally? It's sort of non-trivial to match a message property to the format of a parameter of a separate RPC method.

codyoss commented 3 years ago

protobuf message and has no format annotation. It has the same format - but would that hold universally

I am not sure if it holds, but this was a hunch I had about getting it in a parsable format. I don't believe message body's ever directly include any type of format annotation.

It's sort of non-trivial to match a message property to the format of a parameter of a separate RPC method

Acknowledged, yeah this option might only want/need to be explored if trying to parse out a format with a heuristic is not producing reliable results.

toumorokoshi commented 1 year ago

Hello! for context: I'm working at Google on the AIPs.

We would love to build/parse this structure automatically from raw values but it's hard to automate this without extra metadata in addition to the textual description.

Is there a concrete example of how this would work? the two examples I see are:

  1. the machineType for a GCP zone. In this case, would you read the zone from the instance, then assume that the desired instance type is supported in that zone?

It also seems slightly brittle to me for a provider to make assumptions about how to construct that string, and attempt to wrap over it: someone would look at the API docs and be confused about how to express the same concept in the Pulumi provider, or vice versa.

For 2: the name field is a common convention for Google APIs (link).

At least for cloud functions, I see there is a flatPath parameter that mostly looks like what you're looking for (sans the v2 prefix):

   "get": {
                  "parameterOrder": [
                    "name"
                  ],
                  "path": "v2/{+name}",
                  "response": {
                    "$ref": "Function"
                  },
                  "scopes": [
                    "https://www.googleapis.com/auth/cloud-platform"
                  ],
                  "description": "Returns a function with the given name from the requested project.",
                  "id": "cloudfunctions.projects.locations.functions.get",
                  "httpMethod": "GET",
                  "flatPath": "v2/projects/{projectsId}/locations/{locationsId}/functions/{functionsId}",
                  "parameters": {
                    "name": {
                      "location": "path",
                      "type": "string",
                      "required": true,
                      "description": "Required. The name of the function which details should be obtained.",
                      "pattern": "^projects/[^/]+/locations/[^/]+/functions/[^/]+$"
                    }
                  }

https://cloudfunctions.googleapis.com/$discovery/rest?version=v2.

There's also pattern actually.

mikhailshilkov commented 1 year ago

Hi @toumorokoshi! Thank you for looking into this!

Is there a concrete example of how this would work?

Something like the flatPath example you gave would work. A format string with clear template and named template arguments, corresponding to other input properties.

It also seems slightly brittle to me for a provider to make assumptions about how to construct that string, and attempt to wrap over it

I believe that's what TF does as well. You'd be able to set machine_type = "e2-medium" and they take care of passing the right value to the API. I suppose we could accept both short-hand and full value formats.

At least for cloud functions, I see there is a flatPath parameter that mostly looks like what you're looking for (sans the v2 prefix):

I think our comment refererred to the name being a type property for Create request, not a GET URI parameter, see here: https://github.com/pulumi/pulumi-google-native/blob/14f8663bbc53e1303374f84803352abf6a72279c/discovery/cloudfunctions_v2.json#L842

toumorokoshi commented 1 year ago

Something like the flatPath example you gave would work. A format string with clear template and named template arguments, corresponding to other input properties.

Great! I'd give that a try first. It's a generated field, I believe.

I believe that's what TF does as well. You'd be able to set machine_type = "e2-medium" and they take care of passing the right value to the API. I suppose we could accept both short-hand and full value formats.

I wouldn't necessarily use TF as the golden standard: google-terraform-provider also has a lot of complex code that attempts to improve usability of the APIs, at the cost of additional per-resource complexity that results in an O(resources) maintenance cost that won't go down.

If the aim of the Pulumi native provider is O(1) effort, I'd recommend making the layer as thin as possible and asking for fixes on the API side.

That said for name and parent in particular splitting those out is pretty common: both aggregate the resource identifiers and contain common parameters like location, project, etc.

I think our comment refererred to the name being a type property for Create request, not a GET URI parameter, see here:

The context of "name" doesn't matter so much: it'll be the same string in the Create request as well. It could be we need to replicate that flatPath in all instances where name is mentioned.

That said Create doesn't use that name parameter: it's a bit confusing, but create requests use the parent field (indicating the parent resource) and a resource_id field:

        "create": {
                  "path": "v2/{+parent}/functions",
                  "description": "Creates a new function. If a function with the given name already exists in the specified project, the long running operation will return `ALREADY_EXISTS` error.",
                  "flatPath": "v2/projects/{projectsId}/locations/{locationsId}/functions",
                  "id": "cloudfunctions.projects.locations.functions.create",
                  "parameterOrder": [
                    "parent"
                  ],
                  "scopes": [
                    "https://www.googleapis.com/auth/cloud-platform"
                  ],
                  "parameters": {
                    "parent": {
                      "description": "Required. The project and location in which the function should be created, specified in the format `projects/*/locations/*`",
                      "required": true,
                      "location": "path",
                      "type": "string",
                      "pattern": "^projects/[^/]+/locations/[^/]+$"
                    },
                    "functionId": {
                      "description": "The ID to use for the function, which will become the final component of the function's resource name. This value should be 4-63 characters, and valid characters are /a-z-/.",
                      "location": "query",
                      "type": "string"
                    }
                  },
                  "response": {
                    "$ref": "Operation"
                  },
                  "request": {
                    "$ref": "Function"
                  },
                  "httpMethod": "POST"
                },

See https://google.aip.dev/133#request-message.