Aligns API Fields with K8s Conventions #538

danehans · 2021-02-04T23:45:09Z

What type of PR is this?

/kind bug
/kind api-change

What this PR does / why we need it:
This PR does the following:

Updates API fields to be consistent with Kubernetes API conventions.
Makes httproute.spec.rules optional since all subfields are optional and sets a default value.
Makes the port field of RouteForwardTo optional. When unspecified, the port of the request will be used to forward to a backend. Note: All other RouteForwardTo fields are optional.

Which issue(s) this PR fixes:

Fixes #537

Does this PR introduce a user-facing change?:

- The port field of `RouteForwardTo` is now optional.
- `httproute.spec.rules` is now optional. When unspecified, `kubebuilder:default={{matches: {{path: {type: "Prefix", value: "/"}}}}}` will be used.

xref: kubernetes/enhancements#2366 (comment)

/assign @robscott @hbagdi
/cc @bowei @youngnick @jpeach @stevesloka

k8s-ci-robot · 2021-02-04T23:45:13Z

@danehans: GitHub didn't allow me to request PR reviews from the following users: stevesloka.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind bug
/kind api-change

What this PR does / why we need it:
This PR does the following:

Updates API fields to be consistent with Kubernetes API conventions.

Sets a default for the BackendPolicy status field.

Applies the +kubebuilder:validation:MinLength=1 tag to required string fields. Note: This tag was already applied to some required string fields.

Makes httproute.spec.rules optional since all subfields are optional and sets a default value.

Sets the maximum number of GatewayRefs to 16.

Makes the port field of RouteForwardTo optional. When unspecified, the port of the request will be used to forward to a backend. Note: All other RouteForwardTo fields are optional.

Which issue(s) this PR fixes:

Fixes #537

Does this PR introduce a user-facing change?:
- The port field of `RouteForwardTo` is now optional.
- `httproute.spec.rules` is now optional. When unspecified, `kubebuilder:default={{matches: {{path: {type: "Prefix", value: "/"}}}}}` will be used.
- The maximum number of `GatewayRefs` is now 16.
xref: kubernetes/enhancements#2366 (comment)

/assign @robscott @hbagdi
/cc @bowei @youngnick @jpeach @stevesloka

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-02-04T23:45:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans

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:

~~OWNERS~~ [danehans]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danehans · 2021-02-04T23:48:20Z

@robscott @cmluciano does the validating webhook ensure that a RouteForwardTo includes a serviceName or backendRef value? Note that both fields are optional but one is required to forward the request to the appropriate object.

danehans · 2021-02-04T23:55:58Z

apis/v1alpha1/gateway_types.go

+	// +optional
 	// +kubebuilder:default=Deny
-	Certificate TLSRouteOverrideType `json:"certificate"`
+	Certificate TLSRouteOverrideType `json:"certificate,omitempty"`


+optional/omitempty was added since this field is being defaulted.

danehans · 2021-02-04T23:57:38Z

apis/v1alpha1/gatewayclass_types.go

-//
-// Support: Core.


Fields should indicate the support-level and not the top-level resource.

I think there may be a place for this, especially for route types, but I'm not exactly sure what that should be. I think we want a world where both L4 and L7 implementations can support different parts of this API. I don't mind removing this here though since it doesn't seem like we've added it for other resources.

danehans · 2021-02-04T23:58:55Z

apis/v1alpha1/httproute_types.go

@@ -104,9 +114,10 @@ type HTTPRouteSpec struct {

 	// Rules are a list of HTTP matchers, filters and actions.
 	//
-	// +kubebuilder:validation:MinItems=1
+	// +optional


Set to optional since defaulting is now being used. Note that the default values were already being used in the matches subfield. An HTTPRoute can now be specified without any rules:

kind: HTTPRoute apiVersion: networking.x-k8s.io/v1alpha1 metadata: name: example spec: hostnames: - "foo.com"

After defaulting, the resource will include the default match:

kind: HTTPRoute apiVersion: networking.x-k8s.io/v1alpha1 metadata: name: example spec: hostnames: - "foo.com" rules: - matches: - path: type: Prefix value: /

danehans · 2021-02-05T00:04:12Z

apis/v1alpha1/shared_types.go

 	// +optional
+	// +kubebuilder:validation:MaxItems=16


Is 16 an appropriate maximum number of gatewayRefs?

Good catch, this does feel like an appropriate limit.

@robscott @bowei @jpeach @hbagdi can I get feedback on ^?

I think 16 is higher but I will not object against it strongly.

I can't think of cases where a single route is admitted to more than 16 Gateways.
Even if someone does find a use-case for this, between all kinds of selectors and lists we have in the API, the debugging experience, or even the general UX, of the API would be bad.

There could be some Service Mesh scenarios that would use this. My general preference is to start with lower limits.

@hbagdi thanks for the feedback. I'll reduce the maximum to 8. @howardjohn PTAL and let us know if you have issue with this maximum.

For a service mesh use case I would imagine its either a couple gateways at most, or 1 Gateway per service at which point this would be well beyond 16 and instead be 1000s which wouldn't work before or after this change. Probably the former would be a much more sensible way to model it.

So this seems reasonable

I see MaxItems=16 throughout the API's. I want to avoid having this PR cause a regression, so I'm going to remove this change.

jpeach · 2021-02-05T02:27:08Z

apis/v1alpha1/backendpolicy_types.go

@@ -31,16 +31,25 @@ import (
 // implementation specific may be represented with similar implementation
 // specific custom resources.
 type BackendPolicy struct {
-	metav1.TypeMeta   `json:",inline"`
+	metav1.TypeMeta `json:",inline"`
+	// +optional


why optional here?

@jpeach I'm following k8s api conventions. When a field includes omitempty, it should be tagged +optional.

xref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required
xref: https://github.com/kubernetes/api/blob/master/apps/v1/types.go

Is it that the omitempty tag here is wrong instead?
How can metav1.ObjectMeta possibly be empty for any valid k8s resource?

jpeach · 2021-02-05T02:28:35Z

apis/v1alpha1/backendpolicy_types.go

-	Spec   BackendPolicySpec   `json:"spec,omitempty"`
+	// Spec defines the desired state of BackendPolicy.
+	//
+	// +optional


What's the utility of a resource with an empty spec?

Same as https://github.com/kubernetes-sigs/service-apis/pull/538/files#r571116077.

A potential use case is to use api defaulting for spec. A resource with all defaults can be created as such:

kind: Gateway apiVersion: networking.x-k8s.io/v1alpha1 metadata: name: example

and will result in:

kind: Gateway apiVersion: networking.x-k8s.io/v1alpha1 metadata: name: example spec: gatewayClassName: default listeners: - protocol: HTTP port: 80 routes: kind: HTTPRoute selector: matchLabels: app-type: http-exposed namespaces: from: "Same"

This could simplify the user experience for the basic http use case.

Note: All these defaults above do not exist today.

I wonder whether this UX would be better in a kubectl plugin? I'm a bit skeptical that defaulting at this layer is the right approach.

jpeach · 2021-02-05T02:30:30Z

apis/v1alpha1/backendpolicy_types.go

@@ -102,26 +121,28 @@ type BackendTLSConfig struct {
 	// Support: Extended
 	//
 	// +optional
-	CertificateAuthorityRef *LocalObjectReference `json:"certificateAuthorityRef,omitempty"`
+	CertificateAuthorityRef LocalObjectReference `json:"certificateAuthorityRef,omitempty"`


this is optional, so should still be a pointer field?

xref https://github.com/kubernetes/api/blob/master/apps/v1/types.go#L50-L52 for other native k8s types with optional struct fields.

jpeach · 2021-02-05T02:31:28Z

apis/v1alpha1/gateway_types.go

@@ -34,18 +34,25 @@ import (
 // This ensures that a GatewayClass associated with a Gateway(s) is not
 // deleted while in use.
 type Gateway struct {
-	metav1.TypeMeta   `json:",inline"`
+	metav1.TypeMeta `json:",inline"`
+	// +optional


is object meta really optional?

Yes, according to k8s api conventions, any field with omitempty should be marked as +optional.

xref: https://github.com/kubernetes/api/blob/master/apps/v1/types.go

Are there any object that do this? I don't recall seeing it before.

I have not seen any object omit this field.

jpeach · 2021-02-05T02:32:09Z

apis/v1alpha1/gatewayclass_types.go

@@ -77,7 +81,7 @@ type GatewayClassSpec struct {
 	// Support: Custom
 	//
 	// +optional
-	ParametersRef *LocalObjectReference `json:"parametersRef,omitempty"`
+	ParametersRef LocalObjectReference `json:"parametersRef,omitempty"`


optional should be pointer?

Per https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required:

They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag) or have a built-in nil value (e.g. maps and slices).

Take a look through other areas of Service APIs and k8s native APIs and you will see:

// +optional SomeField SomeFieldCustomType `json:"someField,omitempty"`

They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag) or have a built-in nil value (e.g. maps and slices).

I don't think that ParametersRef has an OK nil value; the empty string fields create ambiguity that could easily be avoided.

I'll revert this change so the PR can proceed.

jpeach · 2021-02-05T02:33:41Z

apis/v1alpha1/httproute_types.go

 	// +kubebuilder:default={type: "Prefix", value: "/"}
 	Path HTTPPathMatch `json:"path,omitempty"`

 	// Headers specifies a HTTP request header matcher.
 	//
 	// +optional
-	Headers *HTTPHeaderMatch `json:"headers"`
+	Headers HTTPHeaderMatch `json:"headers,omitempty"`


optional should be a pointer

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag) or have a built-in nil value (e.g. maps and slices).

I'll revert this change so the PR can proceed.

jpeach

Couple of broad issues for this change

probably meta, spec and status should not be optional; that seems quite surprising to me
optional field should consistently be pointer fields, https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

robscott

Thanks for the work on this @danehans! Some of these changes were rather surprising, but the vast majority seem to line up with API Conventions. I was especially surprised to see all fields with a default value marked as optional. Even stranger to see meta, spec, and status all marked as optional. In all cases, that seems to line up with upstream conventions. Still feels awfully strange though. Will take a closer look soon, but wanted to add some comments from my first run through.

robscott · 2021-02-05T06:57:55Z

apis/v1alpha1/backendpolicy_types.go

 // Route(s) using backends configured by this BackendPolicy.
 type BackendPolicyStatus struct {
 	// Conditions describe the current conditions of the BackendPolicy.
 	//
 	// +listType=map
 	// +listMapKey=type
+	// +kubebuilder:validation:MinItems=1


Do we know that there will always be a condition here? If so, what would it be?

Similar to https://github.com/kubernetes-sigs/service-apis/pull/528/files#r571154934. For example:

status: - conditions: - type: Admitted status: False message: Waiting for controller reason: waiting

Otherwise we make conditions optional and the lack of conditions means the object has not been reconciled by a controller. I prefer to be explicit instead of requiring the user to infer.

Yeh, I agree that there will always be at least one condition.

So to clarify, we're saying that status itself is optional, but if it is specified it must include at least one condition? I think status on BackendPolicy is much less clear than other resources. Controllers have clear ownership/management of Gateways and status on Routes is tied directly to the Gateways. That makes it clear which controller is responsible for setting conditions in each case.

Unfortunately with BackendPolicy we don't have a good answer to that. I don't think we ever had a great answer here, but even the godocs for this status suggest that most conditions should be set on route and not here. Given that, I'd rather mark this as optional than require a condition.

So to clarify, we're saying that status itself is optional, but if it is specified it must include at least one condition?

@robscott yes, that is what the changes in this PR propose. I will revert these changes so the PR can proceed.

robscott · 2021-02-05T07:06:10Z

apis/v1alpha1/backendpolicy_types.go

@@ -55,33 +64,43 @@ type BackendPolicySpec struct {
 	// the oldest BackendPolicy.
 	//
 	// Support: Core
+	//
+	// +kubebuilder:validation:MinItems=1


I'm not sure we want this change. I think we want it to be possible to specify a BackendPolicy without applying it to any specific backends. This could be helpful if you want to provide a standard set of policies that can be applied to different backends, even if not all of them apply to a backend at once.

@robscott if that's the case, then shouldn't this field be optional? I do like your idea for a nil backendRefs, but the docs and field requirements indicate otherwise.

This is similar to my comment on the other issue, but in some cases I think it's helpful to actually show an empty list instead of just omitting the field entirely. I'm curious what the behavior is now, I'm assuming a user can create a BackendPolicy with [] for this field, do they even need to specify that? I don't read anything in the docs that specifically requires a BackendRef to be set, but we have not made it clear enough that empty is a valid option here, that would definitely be good to update.

I think it's helpful to actually show an empty list instead of just omitting the field entirely.

+1. I'm not versed in conventions but from a user's perspective, this is very helpful for some fields.

I'm assuming a user can create a BackendPolicy with [] for this field, do they even need to specify that?

Other than port, all BackendRef fields are required so an empty BackendRefs is not permitted.

robscott · 2021-02-05T07:08:47Z

apis/v1alpha1/backendpolicy_types.go

 	// +optional
-	TLS *BackendTLSConfig `json:"tls,omitempty"`
+	TLS BackendTLSConfig `json:"tls,omitempty"`


Although the API conventions do have an allowance for types with a built-in nil value, it seems like the overwhelming preference is to use pointers for all optional fields if I'm reading the API conventions correctly.

preference is to use pointers for all optional fields

Not all optional fields. For example:

They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag) or have a built-in nil value (e.g. maps and slices).

We are inconsistently using pointers for struct fields.

Yeah, the conventions are a bit confusing here. I completely agree that they allow for non-pointer optional fields in these scenarios, but the points below seem to suggest we should be using pointers for structs:

structs are not omitted from encoder output even where omitempty is specified, which is messy;

having a pointer consistently imply optional is clearer for users of the Go language client, and any other clients that use corresponding types

From an implementer's perspective, checking for a nil pointer to a struct is far less error prone than checking each member in that struct.

@roberthbailey @hbagdi thank you for the feedback. If pointers are used for optional structs, I would like to be consistent across the API's. This means changing spec/status for all resources. For example:

Spec GatewaySpec `json:"spec,omitempty"` // change to: Spec *GatewaySpec `json:"spec,omitempty"`

By doing so, spec/status optionality will differ from most k8s APIs.

I haven't seen any k8s resources using pointers for meta, spec, or status. I'm not sure why that is, I really don't even understand why they're optional, but I'd rather not diverge from the pattern in upstream for these fields.

robscott · 2021-02-05T07:10:13Z

apis/v1alpha1/backendpolicy_types.go

 	// +optional
-	TLS *BackendTLSConfig `json:"tls,omitempty"`
+	TLS BackendTLSConfig `json:"tls,omitempty"`
 }

 // BackendRef identifies an API object within a known namespace that defaults
 // group to core and resource to services if unspecified.


This comment does not agree with the spec below. I think I'd previously assumed this meant these values would be inferred by a controller, but now that we're formally marking both Group and Kind required, I think we should remove this as well.

robscott · 2021-02-05T07:26:06Z

apis/v1alpha1/gatewayclass_types.go

-//
-// Support: Core.


I think there may be a place for this, especially for route types, but I'm not exactly sure what that should be. I think we want a world where both L4 and L7 implementations can support different parts of this API. I don't mind removing this here though since it doesn't seem like we've added it for other resources.

hbagdi · 2021-02-09T16:04:02Z

apis/v1alpha1/backendpolicy_types.go

@@ -31,16 +31,25 @@ import (
 // implementation specific may be represented with similar implementation
 // specific custom resources.
 type BackendPolicy struct {
-	metav1.TypeMeta   `json:",inline"`
+	metav1.TypeMeta `json:",inline"`
+	// +optional


Is it that the omitempty tag here is wrong instead?
How can metav1.ObjectMeta possibly be empty for any valid k8s resource?

hbagdi · 2021-02-09T16:06:30Z

apis/v1alpha1/backendpolicy_types.go

 	// +optional
-	TLS *BackendTLSConfig `json:"tls,omitempty"`
+	TLS BackendTLSConfig `json:"tls,omitempty"`


From an implementer's perspective, checking for a nil pointer to a struct is far less error prone than checking each member in that struct.

hbagdi · 2021-02-09T16:09:25Z

apis/v1alpha1/backendpolicy_types.go

@@ -55,33 +64,43 @@ type BackendPolicySpec struct {
 	// the oldest BackendPolicy.
 	//
 	// Support: Core
+	//
+	// +kubebuilder:validation:MinItems=1


I think it's helpful to actually show an empty list instead of just omitting the field entirely.

+1. I'm not versed in conventions but from a user's perspective, this is very helpful for some fields.

hbagdi · 2021-02-09T16:11:37Z

apis/v1alpha1/gateway_types.go

 	metav1.ObjectMeta `json:"metadata,omitempty"`

+	// Spec defines the desired state of Gateway.
+	//
+	// +optional
 	Spec GatewaySpec `json:"spec,omitempty"`


I think it should be the other way around where we remove the omitempty tag. An object without a spec is going to confuse users and even maintainers.

Are we OK diverging from how upstream defines spec/status/metadata optionality?

xref: https://github.com/kubernetes/api/blob/master/core/v1/types.go

As strange as it is, I think it likely is appropriate to follow what upstream is doing here.

hbagdi · 2021-02-09T16:13:14Z

apis/v1alpha1/httproute_types.go

+	// Spec defines the desired state of HTTPRoute.
+	//
+	// +optional
+	Spec HTTPRouteSpec `json:"spec,omitempty"`


Same comment as before. We should remove omitempty tag from Spec everywhere rather than adding an +optional.

Same response as https://github.com/kubernetes-sigs/service-apis/pull/538/files#r573947867.

hbagdi · 2021-02-09T16:20:22Z

apis/v1alpha1/shared_types.go

 	// +optional
+	// +kubebuilder:validation:MaxItems=16


I think 16 is higher but I will not object against it strongly.

I can't think of cases where a single route is admitted to more than 16 Gateways.
Even if someone does find a use-case for this, between all kinds of selectors and lists we have in the API, the debugging experience, or even the general UX, of the API would be bad.

There could be some Service Mesh scenarios that would use this. My general preference is to start with lower limits.

robscott

@danehans Thanks for all the work on this! There's a lot of great cleanup in here, and I'd love to get it into v0.2.0 if possible. This PR is pretty big though, and it's hard to truly capture all of it in a review. I'm hoping there are some ways to simplify this so it's easier to get in this week. Here are some areas I'd rather see shelved or punted to another PR.

I don't understand why Metadata, Spec, and Status are all optional. I recognize that all k8s APIs have these fields configured with omitempty and +optional tags, but none are pointers. I think mirroring upstream would be ideal here.
I think structs should be pointers when they are optional. As you've pointed out, the API conventions make an allowance for optional maps and slices that are not pointers, but I'm not seeing the same guidance for structs. In many cases I think it would be difficult to understand if a struct was truly empty, at least more difficult than simply checking if the value of the field was nil.
I think a default value for BackendPolicy status does not make sense. I'll add more comments specifically on that field.
In general, I'm uncomfortable with increases to min lengths in this scope because they represent breaking API changes. Although we've been open to breaking changes in our Go API for new minor releases, I don't think we should extend that to our Kubernetes API unless it's in the form of loosening validation.

robscott · 2021-02-10T05:55:20Z

apis/v1alpha1/shared_types.go

+	// using this Route that are not included in the list. An empty list
+	// means the route has not been admitted by any Gateway.
+	//
+	// +kubebuilder:validation:MinItems=1


This is already covered in https://github.com/kubernetes-sigs/service-apis/pull/528/files#r570753913, let's pull it out of this one so we can get this in.

robscott · 2021-02-10T06:00:32Z

apis/v1alpha1/backendpolicy_types.go

+	// Status defines the current state of BackendPolicy.
+	//
+	// +optional
+	// +kubebuilder:default={conditions: {{type: "Admitted", status: "False", message: "Waiting for controller", reason: "Waiting", lastTransitionTime: "1970-01-01T00:00:00Z"}}}


I think we've already discussed this in another thread, but any form of condition on a BackendPolicy is going to be very hard to get right. This is a resource that may be used by zero-many controllers. I think this kind of default value makes sense for resources like Gateway that are clearly waiting for a single controller, but that's not the case here.

danehans · 2021-02-10T18:39:46Z

I don't understand why Metadata, Spec, and Status are all optional. I recognize that all k8s APIs have these fields configured with omitempty, but none of them have +optional tags. Although following API conventions strictly would result in a +optional tag here, I'm just not seeing it anywhere else. Because of that, I'd prefer to leave these fields as they originally were and try to get some clearer guidance on this from apimachinery. There has to be a reason for how this is done, I'm just not seeing it spelled out anywhere.

+optional is used in the apps group APIs: https://github.com/kubernetes/api/blob/master/apps/v1/types.go

In an effort to move the PR forward, I'll remove +optional from all spec/status/meta fields.

I think structs should be pointers when they are optional. As you've pointed out, the API conventions make an allowance for optional maps and slices that are not pointers, but I'm not seeing the same guidance for structs. In many cases I think it would be difficult to understand if a struct was truly empty, at least more difficult than simply checking if the value of the field was nil.

I'll revert structs to pointers.

I think a default value for BackendPolicy status does not make sense. I'll add more comments specifically on that field.

I'll remove the default.

In general, I'm uncomfortable with increases to min lengths in this scope because they represent breaking API changes. Although we've been open to breaking changes in our Go API for new minor releases, I don't think we should extend that to our Kubernetes API unless it's in the form of loosening validation.

I'll remove minLength changes from this PR.

robscott · 2021-02-10T18:46:57Z

+optional is used in the apps group APIs: https://github.com/kubernetes/api/blob/master/apps/v1/types.go
@danehans you're completely right on this one, sorry for the confusion! I edited my comment but was too late. I'm fine with adding +optional tags in to match upstream.

robscott

Thanks for the work on this @danehans! Just a few small things but otherwise LGTM.

robscott · 2021-02-10T21:42:11Z

apis/v1alpha1/httproute_types.go

@@ -608,7 +630,7 @@ type HTTPRouteForwardTo struct {
 	// in HTTPRouteRule.)
 	//
 	// +optional
-	// +kubebuilder:validation:MaxItems=16
+	// +kubebuilder:validation:MaxItems=8


I think we should only tighten validation with new API versions. If we want to do this I think it should be part of v1alpha2.

@robscott done.

robscott · 2021-02-10T21:43:49Z

apis/v1alpha1/shared_types.go

@@ -181,23 +194,25 @@ type RouteGatewayStatus struct {
 	//
 	// +listType=map
 	// +listMapKey=type
+	// +kubebuilder:validation:MinItems=1


Same comment here - I don't think we should tighten validation as part of this release. It does seem like we're compiling an increasing number of changes for v1alpha2 though.

@robscott done.

robscott · 2021-02-10T21:45:49Z

apis/v1alpha1/shared_types.go

+	// Gateways is a list of Gateways that are associated with the route,
+	// and the status of the route with respect to each Gateway. When a
+	// Gateway selects this route, the controller that manages the Gateway
+	// must add an entry to this list when the controller first sees the
+	// route and should update the entry as appropriate when the route is
+	// modified.
+	//
+	// A minimum of 1 and a maximum of 100 Gateways will be represented
+	// in this list. If the list is full, additional Gateways may be
+	// using this Route that are not included in the list. An empty list
+	// means the route has not been admitted by any Gateway.


I think this is likely covered better with #528, can you pull this change out of this PR?

@robscott done.

robscott · 2021-02-10T21:47:49Z

docs-src/guidelines.md

+mechanisms can consistently be implemented across a diverse set of use
+cases.
+
+[1]: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md


I think the way we're treating fields with default values may be slightly unique here and worth a callout. If I'm understanding the approach correctly, fields with default values are considered optional but will not be pointers because they will never truly be empty?

@robscott I've updated, PTAL.

robscott · 2021-02-11T06:03:24Z

Thanks!

/lgtm

k8s-ci-robot assigned hbagdi and robscott Feb 4, 2021

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Feb 4, 2021

k8s-ci-robot requested review from bowei, youngnick and jpeach February 4, 2021 23:45

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2021

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 4, 2021

danehans force-pushed the optional_fields_audit branch from 152d9ee to 185a2fa Compare February 4, 2021 23:52

danehans commented Feb 4, 2021

View reviewed changes

danehans commented Feb 5, 2021

View reviewed changes

jpeach reviewed Feb 5, 2021

View reviewed changes

jpeach suggested changes Feb 5, 2021

View reviewed changes

robscott reviewed Feb 5, 2021

View reviewed changes

danehans force-pushed the optional_fields_audit branch from 185a2fa to 1a5fd74 Compare February 5, 2021 23:40

hbagdi reviewed Feb 9, 2021

View reviewed changes

robscott reviewed Feb 10, 2021

View reviewed changes

danehans force-pushed the optional_fields_audit branch from 1a5fd74 to 6247bff Compare February 10, 2021 18:50

hbagdi added this to the v0.2.0 milestone Feb 10, 2021

robscott reviewed Feb 10, 2021

View reviewed changes

danehans force-pushed the optional_fields_audit branch from 6247bff to f39df1a Compare February 11, 2021 00:55

Aligns API Fields with K8s Conventions

1389c03

danehans force-pushed the optional_fields_audit branch from f39df1a to 1389c03 Compare February 11, 2021 05:31

danehans mentioned this pull request Feb 11, 2021

internal/dag: Initial implementation for gateway-api projectcontour/contour#3278

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021

k8s-ci-robot merged commit 85792a1 into kubernetes-sigs:master Feb 11, 2021

Aligns API Fields with K8s Conventions #538

Aligns API Fields with K8s Conventions #538

Conversation

danehans commented Feb 4, 2021 • edited Loading

k8s-ci-robot commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021

danehans commented Feb 4, 2021

Choose a reason for hiding this comment

danehans Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Feb 4, 2021 •

edited

Loading

danehans Feb 4, 2021 •

edited

Loading

danehans Feb 4, 2021 •

edited

Loading

danehans Feb 10, 2021 •

edited

Loading

danehans Feb 10, 2021 •

edited

Loading

danehans Feb 10, 2021 •

edited

Loading

danehans Feb 5, 2021 •

edited

Loading

robscott left a comment •

edited

Loading