controller-tools icon indicating copy to clipboard operation
controller-tools copied to clipboard

Warn about things that we strongly recommend avoiding

Open DirectXMan12 opened this issue 4 years ago • 8 comments

#317 introduces support for a pattern we generally don't recommend: pointers as map values. This is mainly for a few legacy use cases that folks want to support, but I think we generally want to warn people that this isn't a pattern that we/the kube API conventions generally suggest/support.

We probably want something that looks a lot like our error reporting system, but works adds a "non-fatal" warning instead of a semi-fatal error. I'd expect something like:

pkg.AddWarning(loader.ErrorFromNode(...))

and then print out warnings at the end.

#317 is the only case I know of off the top of my head, but having this infra would prob be useful in the future.

/help /good-first-issue /kind feature /priority important-soon

DirectXMan12 avatar Apr 27 '20 20:04 DirectXMan12

@DirectXMan12: This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-good-first-issue command.

In response to this:

#317 introduces support for a pattern we generally don't recommend: pointers as map values. This is mainly for a few legacy use cases that folks want to support, but I think we generally want to warn people that this isn't a pattern that we/the kube API conventions generally suggest/support.

We probably want something that looks a lot like our error reporting system, but works adds a "non-fatal" warning instead of a semi-fatal error. I'd expect something like:

pkg.AddWarning(loader.ErrorFromNode(...))

and then print out warnings at the end.

#317 is the only case I know of off the top of my head, but having this infra would prob be useful in the future.

/help /good-first-issue /kind feature /priority important-soon

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 avatar Apr 27 '20 20:04 k8s-ci-robot

I would like to give it a try :) /assign

ksaritek avatar May 13 '20 07:05 ksaritek

I created a guestbook kubebuilder init project and added a *ast.StarExpr to guestbook struct

type Env struct {
	A int    `json:"a"`
	B string `json:"b"`
}

// Guestbook is the Schema for the guestbooks API
type Guestbook struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   GuestbookSpec   `json:"spec,omitempty"`
	Status GuestbookStatus `json:"status,omitempty"`
	Envs   map[string]*Env `json:"envs,omitempty"`
}

my patch:

./controller-gen crd object:headerFile="hack/boilerplate.go.txt" paths="./..." output:crd:stdout

---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: (devel)
  creationTimestamp: null
  name: guestbooks.webapp.my.domain
spec:
  group: webapp.my.domain
  names:
    kind: Guestbook
    listKind: GuestbookList
    plural: guestbooks
    singular: guestbook
  scope: Namespaced
  validation:
    openAPIV3Schema:
      description: Guestbook is the Schema for the guestbooks API
      properties:
        apiVersion:
          description: 'APIVersion defines the versioned schema of this representation
            of an object. Servers should convert recognized schemas to the latest
            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
          type: string
        envs:
          additionalProperties:
            properties:
              a:
                type: integer
              b:
                type: string
            required:
            - a
            - b
            type: object
          type: object
        kind:
          description: 'Kind is a string value representing the REST resource this
            object represents. Servers may infer this from the endpoint the client
            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
          type: string
        metadata:
          type: object
        spec:
          description: GuestbookSpec defines the desired state of Guestbook
          properties:
            foo:
              description: Foo is an example field of Guestbook. Edit Guestbook_types.go
                to remove/update
              type: string
          type: object
        status:
          description: GuestbookStatus defines the observed state of Guestbook
          type: object
      type: object
  version: v1
  versions:
  - name: v1
    served: true
    storage: true
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: null
  storedVersions: null
/go/src/example/api/v1/guestbook_types.go:55:20: map values should be a named type, not *ast.StarExpr

controller-gen prints warning at the end like /go/src/example/api/v1/guestbook_types.go:55:20: map values should be a named type, not *ast.StarExpr. @DirectXMan12 , it that right?

ksaritek avatar May 14 '20 22:05 ksaritek

pull request: https://github.com/kubernetes-sigs/controller-tools/pull/443

ksaritek avatar May 21 '20 22:05 ksaritek

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot avatar Aug 19 '20 23:08 fejta-bot

/remove-lifecycle stale

ksaritek avatar Aug 20 '20 07:08 ksaritek

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot avatar Nov 18 '20 07:11 fejta-bot

/remove-lifecycle stale /lifecycle frozen

DirectXMan12 avatar Nov 19 '20 00:11 DirectXMan12