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

Condition type has developer-focused godoc that ends up in CRDs that use the type #169

Closed
Miciah opened this issue Mar 19, 2024 · 5 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@Miciah
Copy link

Miciah commented Mar 19, 2024

The metav1.Condition type definition has the following godoc:

// Condition contains details for one aspect of the current state of this API Resource.
// + ---
// + This struct is intended for direct use as an array at the field path .status.conditions.  For example,
// +
// + 	type FooStatus struct{
// + 	    // Represents the observations of a foo's current state.
// + 	    // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
// + 	    // +patchMergeKey=type
// + 	    // +patchStrategy=merge
// + 	    // +listType=map
// + 	    // +listMapKey=type
// + 	    Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
// +
// + 	    // other fields
// + 	}
type Condition struct {

The Type field has the following godoc:

	// type of condition in CamelCase or in foo.example.com/CamelCase.
	// ---
	// Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
	// useful (see .node.status.conditions), the ability to deconflict is important.
	// The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
	// +required
	// +kubebuilder:validation:Required
	// +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$`
	// +kubebuilder:validation:MaxLength=316
	Type string `json:"type" protobuf:"bytes,1,opt,name=type"`

This information ends up in CRDs that use the Condition type. For example, it can be observed in Gateway API's HTTPRoute API, which uses the Condition type (observe the last part of the output before "FIELDS:"), as well as the output after type <string>; note that I omitted the output for other fields, as denoted by [...]):

% kubectl create -f https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/main/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml
customresourcedefinition.apiextensions.k8s.io/httproutes.gateway.networking.k8s.io created
% kubectl explain httproutes.status.parents.conditions
KIND:     HTTPRoute
VERSION:  gateway.networking.k8s.io/v1

RESOURCE: conditions <[]Object>

DESCRIPTION:
     Conditions describes the status of the route with respect to the Gateway.
     Note that the route's availability is also subject to the Gateway's own
     status conditions and listener status.


     If the Route's ParentRef specifies an existing Gateway that supports Routes
     of this kind AND that Gateway's controller has sufficient access, then that
     Gateway's controller MUST set the "Accepted" condition on the Route, to
     indicate whether the route has been accepted or rejected by the Gateway,
     and why.


     A Route MUST be considered "Accepted" if at least one of the Route's rules
     is implemented by the Gateway.


     There are a number of cases where the "Accepted" condition may not be set
     due to lack of controller visibility, that includes when:


     * The Route refers to a non-existent parent.
     * The Route is of a type that the controller does not support.
     * The Route is in a namespace the controller does not have access to.

     Condition contains details for one aspect of the current state of this API
     Resource. --- This struct is intended for direct use as an array at the
     field path .status.conditions. For example,


     type FooStatus struct{ // Represents the observations of a foo's current
     state. // Known .status.conditions.type are: "Available", "Progressing",
     and "Degraded" // +patchMergeKey=type // +patchStrategy=merge //
     +listType=map // +listMapKey=type Conditions []metav1.Condition
     `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"
     protobuf:"bytes,1,rep,name=conditions"`


     // other fields }

FIELDS:
[...]
   type <string> -required-
     type of condition in CamelCase or in foo.example.com/CamelCase. --- Many
     .condition.type values are consistent across resources like Available, but
     because arbitrary conditions can be useful (see .node.status.conditions),
     the ability to deconflict is important. The regex it matches is
     (dns1123SubdomainFmt/)?(qualifiedNameFmt)

Ideally, the godoc should use kubebuilder syntax to omit the developer-focused portions from generated CRDs:

diff --git a/pkg/apis/meta/v1/types.go b/pkg/apis/meta/v1/types.go
index 9695ba50..395990e0 100644
--- a/pkg/apis/meta/v1/types.go
+++ b/pkg/apis/meta/v1/types.go
@@ -1515,26 +1515,26 @@ type PartialObjectMetadataList struct {
 }
 
 // Condition contains details for one aspect of the current state of this API Resource.
-// ---
-// This struct is intended for direct use as an array at the field path .status.conditions.  For example,
-//
-//	type FooStatus struct{
-//	    // Represents the observations of a foo's current state.
-//	    // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
-//	    // +patchMergeKey=type
-//	    // +patchStrategy=merge
-//	    // +listType=map
-//	    // +listMapKey=type
-//	    Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
-//
-//	    // other fields
-//	}
+// + ---
+// + This struct is intended for direct use as an array at the field path .status.conditions.  For example,
+// +
+// + 	type FooStatus struct{
+// + 	    // Represents the observations of a foo's current state.
+// + 	    // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
+// + 	    // +patchMergeKey=type
+// + 	    // +patchStrategy=merge
+// + 	    // +listType=map
+// + 	    // +listMapKey=type
+// + 	    Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
+// +
+// + 	    // other fields
+// + 	}
 type Condition struct {
 	// type of condition in CamelCase or in foo.example.com/CamelCase.
-	// ---
-	// Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
-	// useful (see .node.status.conditions), the ability to deconflict is important.
-	// The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
+	// + ---
+	// + Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
+	// + useful (see .node.status.conditions), the ability to deconflict is important.
+	// + The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
 	// +required
 	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$`

If this suggestion is acceptable, please let me know, and I will submit a PR. If I am reporting the issue in the wrong place, please let me know where the correct place to report the issue and suggest a change would be.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 17, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 17, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2024
@Miciah
Copy link
Author

Miciah commented Nov 23, 2024

The issue was fixed in a different way by kubernetes-sigs/controller-tools#877:

This PR strips comments from the generated CRDs for clarity of the public API documentation.
[...]
The change aligns controller-tools with Kubernetes conventions for type descriptions:

  • Line that only contains --- separates the API description from comments. The leading lines are part of public documentation, the trailing lines are internal instructions such as code examples etc. As an example, see widely used type Condition that includes some example code that should not be considered as part of API documentation.

For the Gateway API, kubernetes-sigs/gateway-api#3341 regenerated the CRDs using a new controller-tools release that included kubernetes-sigs/controller-tools#877, which did indeed remove the developer-focused text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

3 participants