✨ Warn about things that we strongly recommend avoiding by ksaritek · Pull Request #443 · kubernetes-sigs/controller-tools

ksaritek · 2020-05-21T22:42:36Z

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.

k8s-ci-robot · 2020-05-21T22:42:43Z

Welcome @ksaritek!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-05-21T22:42:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ksaritek
To complete the pull request process, please assign directxman12
You can assign the PR to them by writing /assign @directxman12 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

ksaritek · 2020-05-21T22:46:04Z

/assign @DirectXMan12

DirectXMan12 · 2020-05-27T22:20:13Z

First comment: the beginning of the commit should generally say "what" and "why". Having examples is fine, but it's generally go to snip to the relevant parts, and to used fenced code blocks. I'd expect something like:

We have a set of patterns that we reccomend avoiding (since they're not in
line with the Kubernetes API conventions/best practices), but for legacy
reasons can't make a hard error.  This introduces a mechanism to warn about
such patterns when they're detected.

It functions similarly to the way errors (an `AddWarning` on `Packaged` with
special handling for `PositionedError`), except that such warnings are not
considered fatal at the end of processing.


This commit only introduces one such warning: warn about map values being
pointers.

For example:

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

type Container struct {
    Envs   map[string]*Env `json:"envs,omitempty"`
}
```

yields

```
/path/to/types.go:7:20: map values should be a named type, not *ast.StarExpr
```

DirectXMan12

comments inline. Please remove the merge commits. Usually we prefer one "logical chunk" per commit, so in this case I'd expect to either see 1 commit for the whole thing, or one for the warning functionality and one to update the type error to a warning.

DirectXMan12 · 2020-05-27T22:21:48Z

pkg/crd/flatten.go

+// WarningRecorder knows how to record errors. It wraps the part of
+// pkg/loader.Package that we need to record errors in places were it might not
+// make sense to have a loader.Package


the second sentence here seems like it was copy-pasted verbatim, and doesn't make much sense any more.
I'd go with

Suggested change

// WarningRecorder knows how to record errors. It wraps the part of

// pkg/loader.Package that we need to record errors in places were it might not

// make sense to have a loader.Package

// WarningRecorder knows how to record warnings, which are effectively

// non-fatal errors. It wraps the part of pkg/loader.Package that we need to

// record warnings in places were it might not make sense to have a

// loader.Package.

DirectXMan12 · 2020-05-27T22:22:42Z

pkg/crd/flatten.go

+// pkg/loader.Package that we need to record errors in places were it might not
+// make sense to have a loader.Package
+type WarningRecorder interface {
+	// AddWarning records that the given error occurred.


Suggested change

// AddWarning records that the given error occurred.

// AddWarning records that the given warning (non-fatal error) occurred.

DirectXMan12 · 2020-05-27T22:24:52Z

pkg/crd/schema.go

 			return &apiext.JSONSchemaProps{}
 		}
 	case *ast.StarExpr:
+		ctx.pkg.AddWarning(loader.ErrFromNode(fmt.Errorf("map values should be a named type, not %T", mapType.Value), mapType.Value))


maybe tweak the message here a bit so it's obvious it's a warning. I'd do something like

as per the Kubernetes API conventions, map values *should* generally be primitives or named types that alias primitives, not %T. Consider using the "list as map" pattern here, except in cases where map is used for compatiblity/legacy purposes.

DirectXMan12 · 2020-05-27T22:25:39Z

pkg/loader/loader.go

 	return hadErrors
 }

+// PrintWarnings print errors associated with all packages


Suggested change

// PrintWarnings print errors associated with all packages

// PrintWarnings print warnings associated with all packages

DirectXMan12 · 2020-05-27T22:26:11Z

pkg/loader/loader.go

+// package's dependencies have been visited (postorder).
+// The boolean result of pre(pkg) determines whether
+// the imports of package pkg are visited.
+func visit(pkgs []*Package, pre func(*Package) bool, post func(*Package)) {


do we ever use pre? If not, we can get rid of it.

DirectXMan12 · 2020-05-27T22:27:37Z

pkg/loader/loader.go

+	// Warnings contains any errors encountered querying the metadata
+	// of the package, or while parsing or type-checking its files.


Suggested change

// Warnings contains any errors encountered querying the metadata

// of the package, or while parsing or type-checking its files.

// Warnings contains any warnings ("non-fatal" errors) encountered while type-checking or

// generating output (OpenAPI, etc) from the given package.

DirectXMan12 · 2020-05-27T22:28:32Z

pkg/loader/loader.go

 	}
 }

+// AddWarning adds an error to the warningss associated with the given package.


Suggested change

// AddWarning adds an error to the warningss associated with the given package.

// AddWarning adds a warning (represented as an error) to the warnings associated with the given package.

// It functions nearly-identically to `AddError`, except that it adds to `Package.Warnings`.

fejta-bot · 2020-08-25T23:04:33Z

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 · 2020-08-26T23:39:34Z

/remove-lifecycle stale

fejta-bot · 2020-11-25T00:36:36Z

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 · 2020-12-25T01:21:01Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

fejta-bot · 2021-01-24T02:04:45Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

k8s-ci-robot · 2021-01-24T02:04:51Z

@fejta-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

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.

ksaritek added 3 commits May 15, 2020 00:15

Warn about things that we strongly recommend avoiding #434

3c6894b

Warn about things that we strongly recommend avoiding #434

4f82619

Merge branch '434' of github.com:ksaritek/controller-tools into 434

9e160b4

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 21, 2020

k8s-ci-robot requested review from DirectXMan12 and droot May 21, 2020 22:42

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 21, 2020

ksaritek changed the title ~~(:sparkles:, minor) fix: Warn about things that we strongly recommend avoiding #434~~ ✨ fix: Warn about things that we strongly recommend avoiding #434 May 21, 2020

ksaritek changed the title ~~✨ fix: Warn about things that we strongly recommend avoiding #434~~ ✨ Warn about things that we strongly recommend avoiding #434 May 21, 2020

ksaritek changed the title ~~✨ Warn about things that we strongly recommend avoiding #434~~ (:sparkles:,minor) Warn about things that we strongly recommend avoiding #434 May 21, 2020

k8s-ci-robot assigned DirectXMan12 May 21, 2020

ksaritek changed the title ~~(:sparkles:,minor) Warn about things that we strongly recommend avoiding #434~~ ✨ Warn about things that we strongly recommend avoiding May 21, 2020

ksaritek changed the title ~~✨ Warn about things that we strongly recommend avoiding~~ ✨ Warn about things that we strongly recommend avoiding May 21, 2020

ksaritek mentioned this pull request May 21, 2020

Warn about things that we strongly recommend avoiding #434

Open

DirectXMan12 suggested changes May 27, 2020

View reviewed changes

DirectXMan12 mentioned this pull request Jun 3, 2020

✨ Add support for AllowDangerousTypes crd flag (enables float support) #449

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 25, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 26, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 25, 2020

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 Dec 25, 2020

k8s-ci-robot closed this Jan 24, 2021

-// WarningRecorder knows how to record errors. It wraps the part of
-// pkg/loader.Package that we need to record errors in places were it might not
-// make sense to have a loader.Package
+// WarningRecorder knows how to record warnings, which are effectively
+// non-fatal errors.  It wraps the part of pkg/loader.Package that we need to
+// record warnings in places were it might not make sense to have a
+// loader.Package.

	// AddWarning records that the given error occurred.
	// AddWarning records that the given warning (non-fatal error) occurred.

	// PrintWarnings print errors associated with all packages
	// PrintWarnings print warnings associated with all packages

		// Warnings contains any errors encountered querying the metadata
		// of the package, or while parsing or type-checking its files.

	// AddWarning adds an error to the warningss associated with the given package.
	// AddWarning adds a warning (represented as an error) to the warnings associated with the given package.
	// It functions nearly-identically to `AddError`, except that it adds to `Package.Warnings`.

Conversation

ksaritek commented May 21, 2020

Uh oh!

k8s-ci-robot commented May 21, 2020

Uh oh!

k8s-ci-robot commented May 21, 2020

Uh oh!

ksaritek commented May 21, 2020

Uh oh!

DirectXMan12 commented May 27, 2020

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

fejta-bot commented Aug 25, 2020

Uh oh!

DirectXMan12 commented Aug 26, 2020

Uh oh!

fejta-bot commented Nov 25, 2020

Uh oh!

fejta-bot commented Dec 25, 2020

Uh oh!

fejta-bot commented Jan 24, 2021

Uh oh!

k8s-ci-robot commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants