Enable gochecknoglobals linter

We often use global variables to represent data that is by convention
constant, but that can't be constant because it's mutable. So there's a
few nolint comments for this, but I think it's still good to discourage
folks.

Signed-off-by: Nic Cope <nicc@rk0n.org>

.golangci.yml

@@ -43,7 +43,6 @@ linters:
     - inamedparam
     - gomnd
     - interfacebloat
-    - gochecknoglobals
 
     # Below are linters that lint for things we don't value. Each entry below
     # this line must have a comment explaining the rationale.
@@ -221,6 +220,7 @@ issues:
         - scopelint
         - unparam
         - gochecknoinits
+        - gochecknoglobals
         - containedctx
         - forcetypeassert
 
@@ -235,6 +235,7 @@ issues:
     - path: apis/
       linters:
       - gochecknoinits
+      - gochecknoglobals
 
     # These are performance optimisations rather than style issues per se.
     # They warn when function arguments or range values copy a lot of memory

cmd/crank/beta/convert/deploymentruntime/converter.go

@@ -18,7 +18,6 @@ package deploymentruntime
 
 import (
 	"errors"
-	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -36,8 +35,6 @@ const (
 	errNilControllerConfig = "ControllerConfig is nil"
 )
 
-var timeNow = time.Now()
-
 // controllerConfigToDeploymentRuntimeConfig converts a ControllerConfig to
 // a DeploymentRuntimeConfig.
 func controllerConfigToDeploymentRuntimeConfig(cc *v1alpha1.ControllerConfig) (*v1beta1.DeploymentRuntimeConfig, error) {
@@ -49,7 +46,7 @@ func controllerConfigToDeploymentRuntimeConfig(cc *v1alpha1.ControllerConfig) (*
 		withName(cc.Name),
 		// set the creation timestamp due to https://github.com/kubernetes/kubernetes/issues/109427
 		// to be removed when fixed. k8s apply ignores this field
-		withCreationTimestamp(metav1.NewTime(timeNow)),
+		withCreationTimestamp(metav1.Now()),
 		withServiceAccountTemplate(cc),
 		withServiceTemplate(cc),
 		withDeploymentTemplate(dt),
@@ -80,7 +77,7 @@ func deploymentTemplateFromControllerConfig(cc *v1alpha1.ControllerConfig) *v1be
 	// set the creation timestamp due to https://github.com/kubernetes/kubernetes/issues/109427
 	// to be removed when fixed. k8s apply ignores this field
 	if cc.CreationTimestamp.IsZero() || dt.Spec.Template.ObjectMeta.CreationTimestamp.IsZero() {
-		dt.Spec.Template.ObjectMeta.CreationTimestamp = metav1.NewTime(timeNow)
+		dt.Spec.Template.ObjectMeta.CreationTimestamp = metav1.Now()
 	}
 
 	if cc.Spec.Metadata != nil {

cmd/crank/main.go

@@ -30,7 +30,7 @@ import (
 	"github.com/crossplane/crossplane/internal/version"
 )
 
-var _ = kong.Must(&cli)
+var _ = kong.Must(&cli{})
 
 type (
 	versionFlag string
@@ -58,7 +58,7 @@ func (v verboseFlag) BeforeApply(ctx *kong.Context) error { //nolint:unparam //
 }
 
 // The top-level crossplane CLI.
-var cli struct {
+type cli struct {
 	// Subcommands and flags will appear in the CLI help output in the same
 	// order they're specified here. Keep them in alphabetical order.
 
@@ -76,7 +76,7 @@ var cli struct {
 
 func main() {
 	logger := logging.NewNopLogger()
-	ctx := kong.Parse(&cli,
+	ctx := kong.Parse(&cli{},
 		kong.Name("crossplane"),
 		kong.Description("A command line tool for interacting with Crossplane."),
 		// Binding a variable to kong context makes it available to all commands

cmd/crossplane/core/core.go

@@ -68,9 +68,9 @@ type Command struct {
 
 // KongVars represent the kong variables associated with the CLI parser
 // required for the Registry default variable interpolation.
-var KongVars = kong.Vars{
+var KongVars = kong.Vars{ //nolint:gochecknoglobals // We treat these as constants.
 	"default_registry":   xpkg.DefaultRegistry,
-	"default_user_agent": transport.DefaultUserAgent,
+	"default_user_agent": transport.DefaultUserAgent(),
 }
 
 // Run is the no-op method required for kong call tree

cmd/crossplane/main.go

@@ -46,7 +46,7 @@ type (
 	versionFlag bool
 )
 
-var cli struct {
+type cli struct {
 	Debug debugFlag `help:"Print verbose logging statements." short:"d"`
 
 	Version versionFlag `help:"Print version and quit." short:"v"`
@@ -97,7 +97,7 @@ func main() {
 	// objects.
 	s := runtime.NewScheme()
 
-	ctx := kong.Parse(&cli,
+	ctx := kong.Parse(&cli{},
 		kong.Name("crossplane"),
 		kong.Description("An open source multicloud control plane."),
 		kong.BindTo(logging.NewLogrLogger(zl), (*logging.Logger)(nil)),

cmd/crossplane/rbac/rbac.go

@@ -45,7 +45,7 @@ const (
 
 // KongVars represent the kong variables associated with the CLI parser
 // required for the RBAC enum interpolation.
-var KongVars = kong.Vars{
+var KongVars = kong.Vars{ //nolint:gochecknoglobals // We treat these as constants.
 	"rbac_manage_default_var": ManagementPolicyBasic,
 	"rbac_manage_enum_var": strings.Join(
 		[]string{

internal/controller/apiextensions/composite/composition_transforms.go

@@ -455,7 +455,7 @@ func GetConversionFunc(t *v1.ConvertTransform, from v1.TransformIOType) (func(an
 // The unparam linter is complaining that these functions always return a nil
 // error, but we need this to be the case given some other functions in the map
 // may return an error.
-var conversions = map[conversionPair]func(any) (any, error){
+var conversions = map[conversionPair]func(any) (any, error){ //nolint:gochecknoglobals // We treat this map as a constant.
 	{from: v1.TransformIOTypeString, to: v1.TransformIOTypeInt64, format: v1.ConvertTransformFormatNone}: func(i any) (any, error) {
 		s, ok := i.(string)
 		if !ok {

internal/controller/pkg/revision/runtime.go

@@ -58,6 +58,7 @@ const (
 	tlsClientCertsDir        = "/tls/client"
 )
 
+//nolint:gochecknoglobals // We treat these as constants, but take their addresses.
 var (
 	runAsUser                = int64(2000)
 	runAsGroup               = int64(2000)

internal/controller/rbac/definition/roles.go

@@ -53,6 +53,7 @@ const (
 	suffixFinalizers = "/finalizers"
 )
 
+//nolint:gochecknoglobals // We treat these as constants.
 var (
 	verbsEdit   = []string{rbacv1.VerbAll}
 	verbsView   = []string{"get", "list", "watch"}

internal/controller/rbac/provider/roles/roles.go

@@ -51,6 +51,7 @@ const (
 	pluralLeases     = "leases"
 )
 
+//nolint:gochecknoglobals // We treat these as constants.
 var (
 	verbsEdit   = []string{rbacv1.VerbAll}
 	verbsView   = []string{"get", "list", "watch"}
@@ -66,6 +67,8 @@ var (
 // * ConfigMaps for leader election.
 // * Leases for leader election.
 // * Events for debugging.
+//
+//nolint:gochecknoglobals // We treat this as a constant.
 var rulesSystemExtra = []rbacv1.PolicyRule{
 	{
 		APIGroups: []string{"", coordinationv1.GroupName},

internal/initializer/cert_generator.go

@@ -43,7 +43,7 @@ type CertificateGenerator interface {
 	Generate(*x509.Certificate, *CertificateSigner) (key []byte, crt []byte, err error)
 }
 
-var pkixName = pkix.Name{
+var pkixName = pkix.Name{ //nolint:gochecknoglobals // We treat this as a constant.
 	CommonName:   "Crossplane",
 	Organization: []string{"Crossplane"},
 	Country:      []string{"Earth"},

internal/metrics/metrics.go

@@ -18,7 +18,11 @@ package metrics
 
 import "sigs.k8s.io/controller-runtime/pkg/metrics"
 
+// TODO(negz): Should we try to plumb the metrics registry down to all callers?
+// I think this would be a good practice - similar to how we plumb the logger.
+// On the other hand, using a global metrics registry is idiomatic for Prom.
+
 // Registry is a Prometheus metrics registry. All Crossplane metrics should be
 // registered with it. Crossplane adds metrics to the registry created and
 // served by controller-runtime.
-var Registry = metrics.Registry
+var Registry = metrics.Registry //nolint:gochecknoglobals // See TODO above.

internal/transport/transport.go

@@ -26,7 +26,9 @@ import (
 
 // DefaultUserAgent is the default User-Agent header that is set when making
 // HTTP requests for packages.
-var DefaultUserAgent = fmt.Sprintf("%s/%s", "crossplane", version.New().GetVersionString())
+func DefaultUserAgent() string {
+	return fmt.Sprintf("%s/%s", "crossplane", version.New().GetVersionString())
+}
 
 // UserAgent wraps a RoundTripper and injects a user agent header.
 type UserAgent struct {

internal/xcrd/schemas.go

@@ -29,11 +29,11 @@ const (
 )
 
 // CompositionRevisionRef should be propagated dynamically.
-var CompositionRevisionRef = "compositionRevisionRef"
+const CompositionRevisionRef = "compositionRevisionRef"
 
 // PropagateSpecProps is the list of XRC spec properties to propagate
 // when translating an XRC into an XR.
-var PropagateSpecProps = []string{"compositionRef", "compositionSelector", "compositionUpdatePolicy", "compositionRevisionSelector"}
+var PropagateSpecProps = []string{"compositionRef", "compositionSelector", "compositionUpdatePolicy", "compositionRevisionSelector"} //nolint:gochecknoglobals // We treat this as a constant.
 
 // TODO(negz): Add descriptions to schema fields.
 

test/e2e/funcs/feature.go

@@ -306,7 +306,7 @@ func (nf notFound) String() string { return "NotFound" }
 
 // NotFound is a special 'want' value that indicates the supplied path should
 // not be found.
-var NotFound = notFound{}
+var NotFound = notFound{} //nolint:gochecknoglobals // We treat this as a constant.
 
 // ResourcesHaveFieldValueWithin fails a test if the supplied resources do not
 // have the supplied value at the supplied field path within the supplied