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

Pydantic, exclude_defaults=True re-add apiVersion and kind #114

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

haarchri
Copy link
Contributor

@haarchri haarchri commented Jan 6, 2025

Description of your changes

In Pydantic, exclude_defaults=True in model_dump excludes fields that have their value equal to the default. If a field like
apiVersion is set to its default value 's3.aws.upbound.io/v1beta2' (and not explicitly provided during initialization), it will be excluded from the serialized output.

so we will explicit add apiVersion and kind

Fixes #

I have:

Tests

tested via

dependencies = [
  "click==8.1.7",
  "grpcio==1.66.2",
  "crossplane-function-sdk-python @ git+https://github.com/haarchri/function-sdk-python@test"
]

[tool.hatch.metadata]
allow-direct-references = true

before:

up composition render  apis/xstoragebuckets/composition.yaml examples/storagebucket/example.yaml
---
apiVersion: platform.example.com/v1alpha1
kind: XStorageBucket
metadata:
  name: example
status:
  conditions:
  - lastTransitionTime: "2024-01-01T00:00:00Z"
    message: 'Unready resources: bucket'
    reason: Creating
    status: "False"
    type: Ready
---
metadata:
  annotations:
    crossplane.io/composition-resource-name: bucket
  generateName: example-
  labels:
    crossplane.io/composite: example
  ownerReferences:
  - apiVersion: platform.example.com/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: XStorageBucket
    name: example
    uid: ""
spec:
  forProvider:
    region: us-west-1

with this PR:

up composition render  apis/xstoragebuckets/composition.yaml examples/storagebucket/example.yaml
---
apiVersion: platform.example.com/v1alpha1
kind: XStorageBucket
metadata:
  name: example
status:
  conditions:
  - lastTransitionTime: "2024-01-01T00:00:00Z"
    message: 'Unready resources: bucket'
    reason: Creating
    status: "False"
    type: Ready
---
apiVersion: s3.aws.upbound.io/v1beta1
kind: Bucket
metadata:
  annotations:
    crossplane.io/composition-resource-name: bucket
  generateName: example-
  labels:
    crossplane.io/composite: example
  ownerReferences:
  - apiVersion: platform.example.com/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: XStorageBucket
    name: example
    uid: ""
spec:
  forProvider:
    region: us-west-1

Comment on lines +41 to +49
data = source.model_dump(exclude_defaults=True, warnings=False)
# In Pydantic, exclude_defaults=True in model_dump excludes fields
# that have their value equal to the default. If a field like
# apiVersion is set to its default value 's3.aws.upbound.io/v1beta2'
# (and not explicitly provided during initialization), it will be
# excluded from the serialized output.
data['apiVersion'] = source.apiVersion
data['kind'] = source.kind
r.resource.update(data)
Copy link
Member

Choose a reason for hiding this comment

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

I don't love this workaround but I can't think of a better option.

I used exclude_defaults to stop model_dump from serializing optional fields as explicit None values. We could use exclude_none instead, but I think apart from apiVersion and kind we really don't want to include defaults - since we want to leave the defaulting to server-side apply.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, how feasible would it be to do the following?

  1. Update the models to remove all default values (except for apiVersion and kind)
  2. Switch to exclude_none

Copy link
Contributor Author

Choose a reason for hiding this comment

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

people will lose the information which default values are available - in lsp - mhm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apiVersion and kind are special - so i don't think the solution at all is a problem

@negz
Copy link
Member

negz commented Jan 8, 2025

Overriding failing tests - they need to be fixed in main.

@negz negz merged commit d4b3506 into crossplane:main Jan 8, 2025
5 of 6 checks passed
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.

2 participants