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

revert leaving optional for required init fields #257

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

lsviben
Copy link
Contributor

@lsviben lsviben commented Aug 17, 2023

Description of your changes

Related to crossplane-contrib/provider-upjet-gcp#355 .

In #237, the required fields in spec.forProvider were set as optional due to them being replacable by the fields in spec.initProvider (just the fields that are in spec.initProvider not all required fields like identifiers).

This also added, or rather didnt remove, the omitempty json tag for those fields. This causes boolean fields to not be properly late-initialized, which in turn causes the resource to be stuck in Synced: FALSE status as it does not have all the required fields.

So this PR reverts the change in which the omitempty tag was not being removed for fields that are required && initProvider field. Instead it removes the omitempty for all fields that are required.

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Ran make generate on provider-gcp and tested running locally with a kind cluster.

The resource created is the cluster from the bug issue: crossplane-contrib/provider-upjet-gcp#355 (comment)

apiVersion: container.gcp.upbound.io/v1beta1
kind: Cluster
metadata:
  annotations:
    meta.upbound.io/example-id: container/v1beta1/cluster
  labels:
    testing.upbound.io/example-name: cluster
  name: cluster
spec:
  forProvider:
    location: europe-north1
    ipAllocationPolicy:
      - {}
    enableAutopilot: true

The result is:

kubectl get -f cluster.yaml 
NAME      READY   SYNCED   EXTERNAL-NAME   AGE
cluster   True    True     cluster         89m

Copy link
Member

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Thanks @lsviben 💪

@lsviben
Copy link
Contributor Author

lsviben commented Aug 17, 2023

@ulucinar @sergenyalcin

would adding a label according to Added backport release-x.y labels to auto-backport this PR if necessary. work in this repo? Ideally this would be backported and then patched to the provider releases

Copy link
Contributor

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants