kubernetes-sigs / controller-tools

Tools to use with the controller-runtime libraries
Apache License 2.0
706 stars 408 forks source link

Warn about things that we strongly recommend avoiding #434

Open DirectXMan12 opened 4 years ago

DirectXMan12 commented 4 years ago

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

k8s-ci-robot commented 4 years ago

@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](https://github.com/kubernetes-sigs/controller-tools/issues/434): >#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: > >```go >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ksaritek commented 4 years ago

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

ksaritek commented 4 years ago

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 commented 4 years ago

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

fejta-bot commented 3 years ago

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

ksaritek commented 3 years ago

/remove-lifecycle stale

fejta-bot commented 3 years ago

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

DirectXMan12 commented 3 years ago

/remove-lifecycle stale /lifecycle frozen