Hackathon test api review for api change from *string to string #27
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
The change from *string to string for the Hostname field has implications. Changing from a pointer to a value type changes the semantic meaning of the Hostname field.
richabanker
changed the title
richabanker
changed the title
richabanker
force-pushed
the
hackathon
branch
6 times, most recently
from
4c04c6a to
a311a42
Compare
richabanker
force-pushed
the
hackathon-test-api-review-3
branch
from
c6f1d41 to
267ceba
Compare
richabanker
force-pushed
the
hackathon
branch
from
e330f26 to
29df677
Compare
richabanker
force-pushed
the
hackathon-test-api-review-3
branch
from
267ceba to
a0c618b
Compare
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
Hostname should be a pointer to a string. Optional fields should be pointers to distinguish between unset and empty string. Also add +optional tag. e.g. Hostname *string json:"hostname,omitempty" +optional
richabanker
force-pushed
the
hackathon
branch
from
4f7dbdb to
6b1984c
Compare
richabanker
force-pushed
the
hackathon-test-api-review-3
branch
from
a0c618b to
8c5fce0
Compare
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
This field should use a pointer *string to distinguish between an unset value and an empty string. Additionally, include the omitempty tag to omit the field from JSON encoding when empty. It also needs a +optional tag.
richabanker
force-pushed
the
hackathon
branch
from
5fd75a7 to
a933833
Compare
richabanker
force-pushed
the
hackathon-test-api-review-3
branch
from
8c5fce0 to
115e7b3
Compare
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
Consider making Hostname a pointer (*string) to differentiate between an unset value and an empty string. This makes the field optional and backwards compatible. Also add the +optional tag.
richabanker
force-pushed
the
hackathon
branch
from
039d256 to
99ea87f
Compare
richabanker
force-pushed
the
hackathon-test-api-review-3
branch
from
115e7b3 to
4d27e14
Compare
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
Suggest using *string instead of string to make the Hostname field optional and avoid sending an empty string when the value is unset.
| @@ -82,7 +82,7 @@ type Endpoint struct { | |||
| // fungible (e.g. multiple A values in DNS). Must pass DNS Label (RFC 1123) | |||
| // validation. | |||
| // +optional | |||
| Hostname *string | |||
| Hostname string | |||
There was a problem hiding this comment.
Consider providing a default value for the Hostname field.
richabanker
force-pushed
the
hackathon
branch
2 times, most recently
from
2c94705 to
a535d8c
Compare
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: