Linter Rule Wishlist

[JoelSpeed]

This issue is intended to track the various linter rules, based on API conventions and API reviewer experience, that we wish to implement within KAL

• commentstart - Godoc for fields within structs should start with the serialised JSON tag name
• jsontags - Each field should have a json tag, and the field name should be camelCase
• optionalorrequired - Each field should have either an optional, or a required marker
• [add statussubresource lint check #2] statussubresource - (CRD - not default) If a struct is marked as // +kubebuilder:object:root=true and contains a status field, it should also have // +kubebuilder:subresource:status
• [Add Conditions linter to verify conditions type, tags and markers #6] conditions
  • Should be the first item in the struct (general idiom - configuration to disable)
  • Should have +listType=map, +listMapKey=type, +patchStrategy=merge, +patchMergeKey=type and +optional
  • Tag should match json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"
    • This is true for built-ins, but not for CRDs, should be configurable to allow skipping protobuf
• [Adds nophase linter #5] nophase - phase fields are deprecated and conditions should be preferred, avoid phase like enum fields
• [Add integers linter to enforce int32/int64 #8] integers
  • No int, use int32 or int64, prefer int32 unless explicitly needed (check bounds and suggest int32)
  • No unsigned integers, validate non-negative integer
• [Add NoBools linter as an opt-in check to prevent usage of boolean types #9] nobools - (Strong opinion - not on by default) - They don't age well, should be enum instead.
• [Add linter rule to check for required fields #3] requiredfields
  • Do not have omitempty
  • Are not pointers
• [Add maxlength linter for strings and arrays #13] maxlength - Strings and Arrays must set maxLength/maxItems (CEL cost limitation)
  • Some string types are excepted, where their maximum length is known (enum, formats: datetime, date, duration)
• New Linter: nofloats #19
  • Floating point values should be avoided as much as possible, never in spec, cannot be reliably round-tripped.
• New Linter: nomaps #16
  • No maps of subobjects should be allowed, allow map[string]string] as this is used for labels, prefer lists of subobjects with names/ids
• New Linter: duplicatemarkers #29
  • Check for duplicate markers on a struct/field, and suggest to deduplicate
• New Linter: statusoptional #31
  • All first children within status should be optional, second level children may be required (eg type within conditions)
• New Linter: optionalfields #21
  • @JoelSpeed disagrees with some of the conventions in upstream here, will need to discuss with upstream, below are what I think we should have. Pattern that lead to exceptions around optional fields seems discouraged now.
  • Should have omitempty (will need to toggle this, or somehow allow exceptions)
  • If field is a struct, and has omitempty, field must a pointer (json can't omit refs to structs)
  • Should not use pointer for maps and slices (difference between 0 length and omitted is likely an anti-pattern?)
  • For strings, if not a pointer, must have a minLength (0 length would be omitted on a round trip)
  • Require all optional fields be points (would like to toggle this, we don't enforce in OpenShift)
• New Linter: timestamps #26
  • Do not use stamp in field names, eg fooTimestamp, use fooTime instead
• New Linter: ssatags #32
  • All arrays must be marked with listType either atomic, set or map
  • For maps, must set at least one listMapKey marker
  • Optional - forbid usage of listType set - issues with SSA and removing items from the list
• New Linter: enums #17
  • Enumerated fields should use type aliases with +enum.
  • TODO: Uncertain about how this plays into CRDs, need to understand this first
  • TODO: Check origin of this, what is the alternative
  • Enum values should be PascalCase
  • Allow exceptions for command-line executable where the proper name is consistent - configurable allowlist
• New Linter: numericbounds #18
  • All numeric fields should be bounds checked, both for too-small or negative, and for too-large
  • TODO: How is this checked in a built-in type? Is there a marker?
  • int64 must be -(2^53) < x < (2^53) or should be represented as a string (javascript float overflows otherwise)
• New Linter: discriminatedunions #20
  • Identify structs marked with +union
  • Should have 1 required field with +uniondiscriminator
  • Other fields should be optional and marked with +unionMember or +unionMember,optional
  • For CRDs, should have a CEL validation rule has(self.<discriminator> && self.<discriminator> == "<member>" ? has(self.member) : !has(self.member) or equivalent if the member is optional
  • Should not have non-member fields present (strong opinion, so, configurable)
• New Linter: defaults #22
  • Should not use +kubebuilder:default and prefer upstream marker
  • Allow preferred marker, per optionalorrequired
• New Linter: markertypos #23
  • Check for common typos
  • eg some markers seem to require = and some require := (more research required)
  • Make sure +kubebuilder and not + kubebuilder
• New Linter: nodurations #24
  • Durations should be avoided and should instead use fooSeconds, avoids having to implement go duration parsing
• New Linter: references #25
  • Fields should use Ref/Refs and not Reference/References
  • For OpenShift, should not include Ref/Refs - a configuration option
• New Linter: nameformats (CRD only) #27
  • For CRD - not default
  • Common fields like name and namespace should be validated with CEL to match DNS1123 subdomain and label respectively
• New Linter: arrayofstruct #28
  • If you have an array of a struct type, the struct must have at least one required field.
  • Prevents the problem seen in NetworkPolicy where adding or removing a single “-” from the YAML representation wildly changes the meaning of an object… (“Match all packets to 10.0.0.1, port 80” vs “Match all packets to 10.0.0.1 on any port, and also match all packets to port 80 on any IP”) I don’t know if other people have run into this and consider it a problem…)
• New Linter: typeandobjectmeta #30
  • Look for top level runtime objects (ie Kinds) and ensure that they have inline typemeta, objectmeta is optional/omitempty, spec is required, status is optional, none of the fields are pointers

Thinking also about the idea of presets (golangci-lint has these) where you could select a preset config for Kubernetes, CRD, OpenShift or others in the future if they have variations. The presets would configure different defaults for the configuration, e.g. setting which linters are on by default, and where there are variations in individual rule config, setting those defaults as well

[everettraven]

@JoelSpeed Just a note that you can spin all the checklist items into individual issues pretty easily with a little icon to the right of each entry. Might be easier to co-ordinate by assigning individual issues that map to each desired linter?