-
Notifications
You must be signed in to change notification settings - Fork 113
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
CRD usage of pointers vs non-pointers #945
CRD usage of pointers vs non-pointers #945
Conversation
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.
@adambkaplan @gabemontero @HeavyWombat Looking for other opinions on optional fields. Imo we should use pointers for objects and fields where we must distinguish the "default value (""
/false
/0
) and nil
. In other cases we should use omitempty
.
@SaschaSchwarze0 Thanks.
I saw this line in api-conventions and decided to use the pointers for primitive types as well. |
I see. Okay. Let's wait for further feedback and if needed bring it up in next Monday's community meeting. |
24d6360
to
b4fea27
Compare
b4fea27
to
b9853c8
Compare
/hold Adding hold so that we merge this after the 0.7.0 release. We should merge this then soon and do the necessary changes in the CLI. |
/unhold @shahulsonhal this needs a rebase |
b9853c8
to
aa2c3d8
Compare
9a679f6
to
922848f
Compare
a03d416
to
6f28bb2
Compare
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 recently learned that the functions with Ptr
suffix in the pointer package are deprecated. That's most of my comments. A few others. If you do not have time for it @shahulsonhal, you can also grant me write access to your fork, I'll then append a commit.
0358d7e
to
a7554c5
Compare
2921259
to
ca71729
Compare
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SaschaSchwarze0 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields. Fixes shipwright-io#397 Update required custom resource fields to non-pointer Update optional custom resource fields that do not have a built-in nil value to pointers
ca71729
to
30f0b0b
Compare
We decided to merge this after the v0.8 release so that we have enough time to adopt the CLI and test things there. |
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.
/lgtm
Now that v0.8.0 is out, we can merge this.
Changes
We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields.
Fixes #397
Update required custom resource fields to non-pointer
Update optional custom resource fields that do not have a built-in nil value to pointers
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes