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

Change .metadata.name to optional for all Patch resources #2126

Merged
merged 3 commits into from Aug 9, 2022

Conversation

lblackstone
Copy link
Member

Proposed changes

This is a minor breaking change that updates the .metadata.name field from required to optional for all Patch resources. This resolves an issue where a nested reference to the ObjectMeta type also required the name field to be set (e.g., for Deployment, StatefulSet, etc.). Instead of requiring the name field in the SDKs, this requirement will be enforced by the provider. This change trades some clarity in the API usage for semantic correctness.

Related issues (optional)

Fix #2115

This is a minor breaking change that updates the `.metadata.name` field from required to optional for all Patch resources. This resolves an issue where a nested reference to the ObjectMeta type also required the name field to be set (e.g., for Deployment, StatefulSet, etc.). Instead of requiring the name field in the SDKs, this requirement will be enforced by the provider. This change trades some clarity in the API usage for semantic correctness.
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@viveklak
Copy link
Contributor

viveklak commented Aug 9, 2022

This is a minor breaking change

I am not sure this is a breaking change since existing code should still work properly?

this requirement will be enforced by the provider

Should we roll that into this PR as well?

@lblackstone
Copy link
Member Author

I am not sure this is a breaking change since existing code should still work properly?

It changes types, which affects Go, C#, and Java. I'd consider this a minor break still since the feature is opt-in.

@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@@ -1262,6 +1262,12 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (
oldInputs := propMapToUnstructured(olds)
newInputs := propMapToUnstructured(news)

if k.serverSideApplyMode && strings.HasSuffix(urn.Type().String(), "Patch") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: would be good to pull strings.HasSuffix(urn.Type().String(), "Patch") to a function like isPatchResource() or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed -- I'll follow up on that in a later change.

@lblackstone lblackstone merged commit fa399ca into master Aug 9, 2022
@pulumi-bot pulumi-bot deleted the lblackstone/patch-name branch August 9, 2022 22:54
@viveklak
Copy link
Contributor

viveklak commented Aug 9, 2022

It changes types, which affects Go, C#, and Java. I'd consider this a minor break still since the feature is opt-in.

I think in general compilation breaks are ok as long as replacements etc. arent triggered. As you said, the point is moot here since its in preview.

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.

DeploymentPatch should not require .spec.template.metadata.name
2 participants