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

Fix required input props in Go SDK #1090

Merged
merged 2 commits into from
May 1, 2020
Merged

Conversation

lblackstone
Copy link
Member

Proposed changes

The generated schema was not setting required input properties
correctly. This change fixes the schema and regenerates the Go
SDK. Many properties have changed to be non-pointer types.

The generated schema was not setting required input properties
correctly. This change fixes the schema and regenerates the Go
SDK. Many properties have changed to be non-pointer types.

Co-authored-by: Mikhail Shilkov <github@mikhail.io>
provider/pkg/gen/schema.go Outdated Show resolved Hide resolved
@lblackstone lblackstone marked this pull request as draft May 1, 2020 01:48
@lblackstone
Copy link
Member Author

Ok, now the schema matches the OpenAPI spec.

OpenAPI v1.18

"io.k8s.api.apps.v1.DeploymentCondition": {
  "description": "DeploymentCondition describes the state of a deployment at a certain point.",
  "properties": {
    "lastTransitionTime": {
      "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Time",
      "description": "Last time the condition transitioned from one status to another."
    },
    "lastUpdateTime": {
      "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Time",
      "description": "The last time this condition was updated."
    },
    "message": {
      "description": "A human readable message indicating details about the transition.",
      "type": "string"
    },
    "reason": {
      "description": "The reason for the condition's last transition.",
      "type": "string"
    },
    "status": {
      "description": "Status of the condition, one of True, False, Unknown.",
      "type": "string"
    },
    "type": {
      "description": "Type of deployment condition.",
      "type": "string"
    }
  },
  "required": [
    "type",
    "status"
  ],
  "type": "object"
},

Pulumi schema

"kubernetes:apps/v1:DeploymentCondition": {
    "description": "DeploymentCondition describes the state of a deployment at a certain point.",
    "properties": {
        "lastTransitionTime": {
            "type": "string",
            "description": "Last time the condition transitioned from one status to another."
        },
        "lastUpdateTime": {
            "type": "string",
            "description": "The last time this condition was updated."
        },
        "message": {
            "type": "string",
            "description": "A human readable message indicating details about the transition."
        },
        "reason": {
            "type": "string",
            "description": "The reason for the condition's last transition."
        },
        "status": {
            "type": "string",
            "description": "Status of the condition, one of True, False, Unknown."
        },
        "type": {
            "type": "string",
            "description": "Type of deployment condition."
        }
    },
    "type": "object",
    "required": [
        "status",
        "type"
    ]
}

@lblackstone lblackstone marked this pull request as ready for review May 1, 2020 02:16
@lblackstone lblackstone requested a review from pgavlin May 1, 2020 02:17
@mikhailshilkov
Copy link
Member

@lblackstone The example you gave with DeploymentCondition works with both yours and my change, that's good.

However, if I generate the schema with your change, kubernetes:apps/v1:Deployment has no required fields:

"kubernetes:apps/v1:Deployment": {
    "description": "Deployment enables declarative updates for Pods and ReplicaSets.\n\nThis resource waits until its status is ready before registering success\nfor create/update, and populating output properties from the current state of the resource.\nThe following conditions are used to determine whether the resource creation has\nsucceeded or failed:\n\n1. The Deployment has begun to be updated by the Deployment controller. If the current\n   generation of the Deployment is \u003e 1, then this means that the current generation must\n   be different from the generation reported by the last outputs.\n2. There exists a ReplicaSet whose revision is equal to the current revision of the\n   Deployment.\n3. The Deployment's '.status.conditions' has a status of type 'Available' whose 'status'\n   member is set to 'True'.\n4. If the Deployment has generation \u003e 1, then '.status.conditions' has a status of type\n   'Progressing', whose 'status' member is set to 'True', and whose 'reason' is\n   'NewReplicaSetAvailable'. For generation \u003c= 1, this status field does not exist,\n   because it doesn't do a rollout (i.e., it simply creates the Deployment and\n   corresponding ReplicaSet), and therefore there is no rollout to mark as 'Progressing'.\n\nIf the Deployment has not reached a Ready state after 10 minutes, it will\ntime out and mark the resource update as Failed. You can override the default timeout value\nby setting the 'customTimeouts' option on the resource.",
    "properties": {
        "apiVersion": {
            "type": "string",
            "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources",
            "const": "apps/v1"
        },
        "kind": {
            "type": "string",
            "description": "Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds",
            "const": "Deployment"
        },
        "metadata": {
            "$ref": "#/types/kubernetes:meta/v1:ObjectMeta",
            "description": "Standard object metadata."
        },
        "spec": {
            "$ref": "#/types/kubernetes:apps/v1:DeploymentSpec",
            "description": "Specification of the desired behavior of the Deployment."
        },
        "status": {
            "$ref": "#/types/kubernetes:apps/v1:DeploymentStatus",
            "description": "Most recently observed status of the Deployment."
        }
    },
    "type": "object"
}

while mine adds

"type": "object",
"required": [
    "apiVersion",
    "kind",
    "metadata",
    "spec",
    "status"
]

This causes the change

type Deployment struct {
	pulumi.CustomResourceState

	// APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
-	ApiVersion pulumi.StringPtrOutput `pulumi:"apiVersion"`
+	ApiVersion pulumi.StringOutput `pulumi:"apiVersion"`
	// Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
-	Kind pulumi.StringPtrOutput `pulumi:"kind"`
+	Kind pulumi.StringOutput `pulumi:"kind"`
	// Standard object metadata.
-	Metadata metav1.ObjectMetaPtrOutput `pulumi:"metadata"`
+	Metadata metav1.ObjectMetaOutput `pulumi:"metadata"`
	// Specification of the desired behavior of the Deployment.
-	Spec DeploymentSpecPtrOutput `pulumi:"spec"`
+	Spec DeploymentSpecOutput `pulumi:"spec"`
	// Most recently observed status of the Deployment.
-	Status DeploymentStatusPtrOutput `pulumi:"status"`
+	Status DeploymentStatusOutput `pulumi:"status"`
}

and a similar change in the .NET SDK.

Doesn't this look correct to you? Isn't ApiVersion always returned from a resource?

@lblackstone
Copy link
Member Author

@mikhailshilkov Yes, I was surprised to discover that those fields are not marked as required in the OpenAPI spec, so this is correct.

"io.k8s.api.apps.v1.Deployment": {
  "description": "Deployment enables declarative updates for Pods and ReplicaSets.",
  "properties": {
    "apiVersion": {
      "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources",
      "type": "string"
    },
    "kind": {
      "description": "Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds",
      "type": "string"
    },
    "metadata": {
      "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta",
      "description": "Standard object metadata."
    },
    "spec": {
      "$ref": "#/definitions/io.k8s.api.apps.v1.DeploymentSpec",
      "description": "Specification of the desired behavior of the Deployment."
    },
    "status": {
      "$ref": "#/definitions/io.k8s.api.apps.v1.DeploymentStatus",
      "description": "Most recently observed status of the Deployment."
    }
  },
  "type": "object",
  "x-kubernetes-group-version-kind": [
    {
      "group": "apps",
      "kind": "Deployment",
      "version": "v1"
    }
  ]
}

Copy link
Member

@mikhailshilkov mikhailshilkov left a comment

Choose a reason for hiding this comment

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

Understood. I'll have to handle this on codegen side then. LGTM.

@lblackstone lblackstone merged commit 9ea5319 into master May 1, 2020
@pulumi-bot pulumi-bot deleted the lblackstone/go-schema-fix branch May 1, 2020 17:09
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.

None yet

3 participants