Achieve Lint Zero (#9907)

Enables lint checks for missing comments on exported methods/fields. The
code-base is actually really good about commenting exported methods and
packages so this didn't take super long. We can always disable it if it
becomes painful, but I think it can be quite useful to force ourselves
to err on the side of commenting things.

Kept the exclusion for package comments because these throw up more
false positives than they're worth (and don't worry `golint ./...`).

After this change `golint ./...` runs clean except source/sink receiver
names and contexts in test args, where golint is just wrong.

Author: julz

.golangci.yaml

@@ -29,6 +29,10 @@ linters:
     - errcheck
 
 issues:
+  include:
+    # Disable excluding issues about comments from golint.
+    - EXC0002
+
   exclude-rules:
     - path: test # Excludes /test, *_test.go etc.
       linters:
@@ -51,3 +55,8 @@ issues:
       text: "receiver name"
       linters:
         - golint
+
+    # This check has quite a few false positives where there isn't much value in the package comment.
+    - text: "ST1000: at least one file in a package should have a package comment"
+      linters:
+        - stylecheck

pkg/apis/config/features.go

@@ -23,6 +23,7 @@ import (
 	cm "knative.dev/pkg/configmap"
 )
 
+// Flag is a string value which can be either Enabled, Disabled, or Allowed.
 type Flag string
 
 const (

pkg/apis/doc.go

@@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// +k8s:deepcopy-gen=package
+
+// Package apis contains the knative serving and autoscaling apis.
 // Api versions allow the api contract for a resource to be changed while keeping
-// backward compatibility by support multiple concurrent versions
+// backward compatibility by supporting multiple concurrent versions
 // of the same resource
-
-// +k8s:deepcopy-gen=package
 package apis

pkg/apis/serving/k8s_validation.go

@@ -81,6 +81,7 @@ var (
 	)
 )
 
+// ValidateVolumes validates the Volumes of a PodSpec.
 func ValidateVolumes(vs []corev1.Volume, mountedVolumes sets.String) (sets.String, *apis.FieldError) {
 	volumes := make(sets.String, len(vs))
 	var errs *apis.FieldError
@@ -628,6 +629,7 @@ func validateProbe(p *corev1.Probe) *apis.FieldError {
 	return errs
 }
 
+// ValidateNamespacedObjectReference validates an ObjectReference which may not contain a namespace.
 func ValidateNamespacedObjectReference(p *corev1.ObjectReference) *apis.FieldError {
 	if p == nil {
 		return nil
@@ -699,7 +701,7 @@ func ValidatePodSecurityContext(ctx context.Context, sc *corev1.PodSecurityConte
 // being validated.
 type userContainer struct{}
 
-// WithUserContainer notes on the context that further validation or defaulting
+// WithinUserContainer notes on the context that further validation or defaulting
 // is within the context of a user container in the revision.
 func WithinUserContainer(ctx context.Context) context.Context {
 	return context.WithValue(ctx, userContainer{}, struct{}{})
@@ -715,7 +717,7 @@ func WithinSidecarContainer(ctx context.Context) context.Context {
 	return context.WithValue(ctx, sidecarContainer{}, struct{}{})
 }
 
-// Check if we are in the context of a sidecar container in the revision.
+// IsInSidecarContainer checks if we are in the context of a sidecar container in the revision.
 func IsInSidecarContainer(ctx context.Context) bool {
 	return ctx.Value(sidecarContainer{}) != nil
 }

pkg/apis/serving/v1/configuration_lifecycle.go

@@ -70,6 +70,9 @@ func (cs *ConfigurationSpec) GetTemplate() *RevisionTemplateSpec {
 	return &cs.Template
 }
 
+// SetLatestCreatedRevisionName sets the LatestCreatedRevisionName on the
+// revision and sets the ConfigurationConditionReady state to unknown if this
+// does not match the LatestReadyRevisionName.
 func (cs *ConfigurationStatus) SetLatestCreatedRevisionName(name string) {
 	cs.LatestCreatedRevisionName = name
 	if cs.LatestReadyRevisionName != name {
@@ -78,27 +81,36 @@ func (cs *ConfigurationStatus) SetLatestCreatedRevisionName(name string) {
 	}
 }
 
+// SetLatestReadyRevisionName sets the LatestReadyRevisionName on the revision.
+// If this matches the LatestCreatedRevisionName, the
+// ConfigurationConditionReady condition is set to true.
 func (cs *ConfigurationStatus) SetLatestReadyRevisionName(name string) {
 	cs.LatestReadyRevisionName = name
 	if cs.LatestReadyRevisionName == cs.LatestCreatedRevisionName {
 		configCondSet.Manage(cs).MarkTrue(ConfigurationConditionReady)
 	}
 }
 
+// MarkLatestCreatedFailed marks the ConfigurationConditionReady condition to
+// indicate that the Revision failed.
 func (cs *ConfigurationStatus) MarkLatestCreatedFailed(name, message string) {
 	configCondSet.Manage(cs).MarkFalse(
 		ConfigurationConditionReady,
 		"RevisionFailed",
 		"Revision %q failed with message: %s.", name, message)
 }
 
+// MarkRevisionCreationFailed marks the ConfigurationConditionReady condition
+// to indicate the Revision creation failed.
 func (cs *ConfigurationStatus) MarkRevisionCreationFailed(message string) {
 	configCondSet.Manage(cs).MarkFalse(
 		ConfigurationConditionReady,
 		"RevisionFailed",
 		"Revision creation failed with message: %s.", message)
 }
 
+// MarkLatestReadyDeleted marks the ConfigurationConditionReady condition to
+// indicate that the Revision was deleted.
 func (cs *ConfigurationStatus) MarkLatestReadyDeleted() {
 	configCondSet.Manage(cs).MarkFalse(
 		ConfigurationConditionReady,

pkg/apis/serving/v1/configuration_types.go

@@ -73,6 +73,7 @@ const (
 	ConfigurationConditionReady = apis.ConditionReady
 )
 
+// IsConfigurationCondition returns true if the given ConditionType is a ConfigurationCondition.
 func IsConfigurationCondition(t apis.ConditionType) bool {
 	return t == ConfigurationConditionReady
 }

pkg/apis/serving/v1/doc.go

@@ -14,8 +14,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package v1 contains the Serving v1 API types.
-
 // +k8s:deepcopy-gen=package
 // +groupName=serving.knative.dev
+
+// Package v1 contains the Serving v1 API types.
 package v1

pkg/apis/serving/v1/revision_helpers.go

@@ -48,11 +48,16 @@ const (
 	// UserQueueMetricsPortName specifies the port name to use for metrics
 	// emitted by queue-proxy for end user.
 	UserQueueMetricsPortName = "http-usermetric"
+)
 
+const (
+	// AnnotationParseErrorTypeMissing is the value of the Type field for
+	// AnnotationParseErrors which indicate an annotation was missing.
 	AnnotationParseErrorTypeMissing = "Missing"
+
+	// AnnotationParseErrorTypeInvalid is the value of the Type field for
+	// AnnotationParseErrors which indicate an annotation was invalid.
 	AnnotationParseErrorTypeInvalid = "Invalid"
-	LabelParserErrorTypeMissing     = "Missing"
-	LabelParserErrorTypeInvalid     = "Invalid"
 )
 
 const (
@@ -77,13 +82,18 @@ type (
 	RoutingState string
 
 	// +k8s:deepcopy-gen=false
+
+	// AnnotationParseError is the error type representing failures to parse annotations.
 	AnnotationParseError struct {
 		Type  string
 		Value string
 		Err   error
 	}
 
 	// +k8s:deepcopy-gen=false
+
+	// LastPinnedParseError is the error returned when the lastPinned annotation
+	// could not be parsed.
 	LastPinnedParseError AnnotationParseError
 )
 

pkg/apis/serving/v1/route_lifecycle.go

@@ -77,68 +77,98 @@ func (rs *RouteStatus) MarkIngressNotConfigured() {
 		"IngressNotConfigured", "Ingress has not yet been reconciled.")
 }
 
+// MarkTrafficAssigned marks the RouteConditionAllTrafficAssigned condition true.
 func (rs *RouteStatus) MarkTrafficAssigned() {
 	routeCondSet.Manage(rs).MarkTrue(RouteConditionAllTrafficAssigned)
 }
 
+// MarkUnknownTrafficError marks the RouteConditionAllTrafficAssigned condition
+// to indicate an error has occurred.
 func (rs *RouteStatus) MarkUnknownTrafficError(msg string) {
 	routeCondSet.Manage(rs).MarkUnknown(RouteConditionAllTrafficAssigned, "Unknown", msg)
 }
 
+// MarkConfigurationNotReady marks the RouteConditionAllTrafficAssigned
+// condition to indiciate the Revision is not yet ready.
 func (rs *RouteStatus) MarkConfigurationNotReady(name string) {
 	routeCondSet.Manage(rs).MarkUnknown(RouteConditionAllTrafficAssigned,
 		"RevisionMissing",
 		"Configuration %q is waiting for a Revision to become ready.", name)
 }
 
+// MarkConfigurationFailed marks the RouteConditionAllTrafficAssigned condition
+// to indicate the Revision has failed.
 func (rs *RouteStatus) MarkConfigurationFailed(name string) {
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionAllTrafficAssigned,
 		"RevisionMissing",
 		"Configuration %q does not have any ready Revision.", name)
 }
 
+// MarkRevisionNotReady marks the RouteConditionAllTrafficAssigned condition to
+// indiciate the Revision is not yet ready.
 func (rs *RouteStatus) MarkRevisionNotReady(name string) {
 	routeCondSet.Manage(rs).MarkUnknown(RouteConditionAllTrafficAssigned,
 		"RevisionMissing",
 		"Revision %q is not yet ready.", name)
 }
 
+// MarkRevisionFailed marks the RouteConditionAllTrafficAssigned condition to
+// indiciate the Revision has failed.
 func (rs *RouteStatus) MarkRevisionFailed(name string) {
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionAllTrafficAssigned,
 		"RevisionMissing",
 		"Revision %q failed to become ready.", name)
 }
 
+// MarkMissingTrafficTarget marks the RouteConditionAllTrafficAssigned
+// condition to indicate a reference traffic target was not found.
 func (rs *RouteStatus) MarkMissingTrafficTarget(kind, name string) {
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionAllTrafficAssigned,
 		kind+"Missing",
 		"%s %q referenced in traffic not found.", kind, name)
 }
 
+// MarkCertificateProvisionFailed marks the
+// RouteConditionCertificateProvisioned condition to indicate that the
+// Certificate provisioning failed.
 func (rs *RouteStatus) MarkCertificateProvisionFailed(name string) {
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionCertificateProvisioned,
 		"CertificateProvisionFailed",
 		"Certificate %s fails to be provisioned.", name)
 }
 
+// MarkCertificateReady marks the RouteConditionCertificateProvisioned
+// condition to indicate that the Certificate is ready.
 func (rs *RouteStatus) MarkCertificateReady(name string) {
 	routeCondSet.Manage(rs).MarkTrue(RouteConditionCertificateProvisioned)
 }
 
+// MarkCertificateNotReady marks the RouteConditionCertificateProvisioned
+// condition to indicate that the Certificate is not ready.
 func (rs *RouteStatus) MarkCertificateNotReady(name string) {
 	routeCondSet.Manage(rs).MarkUnknown(RouteConditionCertificateProvisioned,
 		"CertificateNotReady",
 		"Certificate %s is not ready.", name)
 }
 
+// MarkCertificateNotOwned changes the RouteConditionCertificateProvisioned
+// status to be false with the reason being that there is an existing
+// certificate with the name we wanted to use.
 func (rs *RouteStatus) MarkCertificateNotOwned(name string) {
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionCertificateProvisioned,
 		"CertificateNotOwned",
 		"There is an existing certificate %s that we don't own.", name)
 }
 
 const (
-	AutoTLSNotEnabledMessage            = "autoTLS is not enabled"
+	// AutoTLSNotEnabledMessage is the message which is set on the
+	// RouteConditionCertificateProvisioned condition when it is set to True
+	// because AutoTLS was not enabled.
+	AutoTLSNotEnabledMessage = "autoTLS is not enabled"
+
+	// TLSNotEnabledForClusterLocalMessage is the message which is set on the
+	// RouteConditionCertificateProvisioned condition when it is set to True
+	// because the domain is cluster-local.
 	TLSNotEnabledForClusterLocalMessage = "TLS is not enabled for cluster-local"
 )
 

pkg/apis/serving/v1alpha1/doc.go

@@ -14,10 +14,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+// +k8s:deepcopy-gen=package
+// +groupName=serving.knative.dev
+
+// Package v1alpha1 contains the v1alpha1 versions of the serving apis.
 // Api versions allow the api contract for a resource to be changed while keeping
 // backward compatibility by support multiple concurrent versions
 // of the same resource
-
-// +k8s:deepcopy-gen=package
-// +groupName=serving.knative.dev
 package v1alpha1

pkg/apis/serving/v1alpha1/register.go

@@ -38,8 +38,10 @@ func Resource(resource string) schema.GroupResource {
 }
 
 var (
+	// SchemeBuilder registers the addKnownTypes function.
 	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes)
-	AddToScheme   = SchemeBuilder.AddToScheme
+	// AddToScheme applies all the stored functions to the scheme.
+	AddToScheme = SchemeBuilder.AddToScheme
 )
 
 // Adds the list of known types to Scheme.

pkg/autoscaler/aggregation/bucketing.go

@@ -56,7 +56,7 @@ type TimedFloat64Buckets struct {
 	windowTotal float64
 }
 
-// Implements stringer interface.
+// String implements the Stringer interface.
 func (t *TimedFloat64Buckets) String() string {
 	return spew.Sdump(t.buckets)
 }

pkg/autoscaler/config/autoscalerconfig/doc.go

@@ -16,4 +16,5 @@ limitations under the License.
 
 // +k8s:deepcopy-gen=package
 
+// Package autoscalerconfig contains the config struct for the autoscaler.
 package autoscalerconfig

pkg/deployment/doc.go

@@ -16,4 +16,5 @@ limitations under the License.
 
 // +k8s:deepcopy-gen=package
 
+// Package deployment manages the deployment config.
 package deployment

pkg/gc/config.go

@@ -30,12 +30,17 @@ import (
 )
 
 const (
+	// ConfigName is the name of the config map for garbage collection.
 	ConfigName = "config-gc"
-	Disabled   = -1
+
+	// Disabled is the value (-1) used by various config map values to indicate
+	// the setting is disabled.
+	Disabled = -1
 
 	disabled = "disabled"
 )
 
+// Config defines the tunable parameters for Garbage Collection.
 type Config struct {
 	// Delay duration after a revision create before considering it for GC
 	StaleRevisionCreateDelay time.Duration
@@ -79,6 +84,7 @@ func defaultConfig() *Config {
 	}
 }
 
+// NewConfigFromConfigMapFunc creates a Config from the supplied ConfigMap func.
 func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.ConfigMap) (*Config, error) {
 	logger := logging.FromContext(ctx)
 	minRevisionTimeout := controller.GetResyncPeriod(ctx)

pkg/gc/doc.go

@@ -15,5 +15,6 @@ limitations under the License.
 */
 
 // +k8s:deepcopy-gen=package
+
 // Package gc manages garbage collection of old resources.
 package gc

pkg/leaderelection/config.go

@@ -20,4 +20,6 @@ import (
 	kle "knative.dev/pkg/leaderelection"
 )
 
+// ValidateConfig re-exports the NewConfigFromConfigMap function from pkg/leaderelection
+// in order for config_test.go to validate the config map.
 var ValidateConfig = kle.NewConfigFromConfigMap

pkg/reconciler/autoscaling/hpa/hpa.go

@@ -47,6 +47,7 @@ type Reconciler struct {
 // Check that our Reconciler implements pareconciler.Interface
 var _ pareconciler.Interface = (*Reconciler)(nil)
 
+// ReconcileKind implements Interface.ReconcileKind.
 func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutoscaler) pkgreconciler.Event {
 	logger := logging.FromContext(ctx)
 	logger.Debug("PA exists")

pkg/reconciler/autoscaling/kpa/kpa.go

@@ -72,6 +72,7 @@ type Reconciler struct {
 // Check that our Reconciler implements pareconciler.Interface
 var _ pareconciler.Interface = (*Reconciler)(nil)
 
+// ReconcileKind implements Interface.ReconcileKind.
 func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutoscaler) pkgreconciler.Event {
 	logger := logging.FromContext(ctx)
 

pkg/reconciler/configuration/configuration.go

@@ -55,6 +55,7 @@ type Reconciler struct {
 // Check that our Reconciler implements configreconciler.Interface
 var _ configreconciler.Interface = (*Reconciler)(nil)
 
+// ReconcileKind implements Interface.ReconcileKind.
 func (c *Reconciler) ReconcileKind(ctx context.Context, config *v1.Configuration) pkgreconciler.Event {
 	logger := logging.FromContext(ctx)
 	recorder := controller.GetEventRecorder(ctx)