-
Notifications
You must be signed in to change notification settings - Fork 117
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
Adjust schema for C# codegen #1088
Conversation
@@ -160,11 +172,21 @@ func PulumiSchema(swagger map[string]interface{}) pschema.PackageSpec { | |||
for _, p := range kind.Properties() { | |||
// JSONSchema type includes `$ref` and `$schema` properties, and $ is an invalid character in | |||
// the generated names. | |||
// TODO: this replacement is going to be deleted once pulumi/pulumi codegen supports masking $-fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final state is that the schema would contain fields named $ref
and $schema
and each language SDK needs to remove $
appropriately.
//objectSpec.Required = append(objectSpec.Required, p.name) | ||
} | ||
|
||
if kind.IsNested() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure that's the right way to fill required properties, but this combination gives zero changes compared to the old SDK. Notice that it gives a lot of changes in the Go SDK. Do you feel those are valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks right to me. Of the properties I checked, it looks like they were required properties that were mistakenly treated as optional before.
@@ -803,7 +803,7 @@ func makeSchemaTypeSpec(prop map[string]interface{}, canonicalGroups map[string] | |||
return pschema.TypeSpec{Type: "string"} | |||
case intOrString: | |||
return pschema.TypeSpec{OneOf: []pschema.TypeSpec{ | |||
{Type: "number"}, | |||
{Type: "integer"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number causes float
s in .NET. integer
looks correct to me.
@@ -15,11 +15,11 @@ type MutatingWebhookConfiguration 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes sense.
@@ -15,11 +15,11 @@ type MutatingWebhookConfiguration 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes sense.
//objectSpec.Required = append(objectSpec.Required, p.name) | ||
} | ||
|
||
if kind.IsNested() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks right to me. Of the properties I checked, it looks like they were required properties that were mistakenly treated as optional before.
I'm going to pull the Go changes into a separate PR just so we've got a clearer history on the bugfix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment: #1090 (comment)
ea665ba
to
ca56290
Compare
I'll open a new PR after Go changes are merged and pulumi/pulumi is updated |
A part of #1084
The .NET codegen is not replaced yet, but it's a step towards. The actual codegen required some changes in pulumi/pulumi.