armadaproject/armada

Is your feature request related to a problem? Please describe. Context values are stored using bare string keys throughout the codebase: // internal/common/auth/common.go:14 const principalKey = "principal" // named constant, but still a plain string type // internal/common/auth/common.go:127 context.WithValue(ctx, "user", principal.GetName()) // internal/common/requestid/interceptors.go:46 context.WithValue(ctx, "requestId", id) All three are plain string keys. If any other package stores a context value under the same string (e.g., "user"), the values silently overwrite each other. Go's recommended pattern is to use an unexported custom type so that keys from different packages can never collide, even if they have the same underlying string value: type contextKey string const principalKey contextKey = "principal" Describe the solution you'd like Define an unexported contextKey type (e.g., in internal/common/auth/ or a shared location) Convert principalKey, the "user" key, and the

Is your feature request related to a problem? Please describe. StartUp() in internal/binoculars/server.go calls os.Exit(-1) on initialization errors (lines 28 and 34) instead of returning an error. This means you can't unit test the function (the test process would just die), and it prevents the caller from doing any cleanup. // Current code (internal/binoculars/server.go) func StartUp(config *configuration.BinocularsConfig) (func(), *sync.WaitGroup) { // ... kubernetesClientProvider, err := cluster.NewKubernetesClientProvider(...) if err != nil { log.Errorf("Failed to connect to kubernetes because %s", err) os.Exit(-1) } authServices, err := auth.ConfigureAuth(config.Auth) if err != nil { log.Errorf("Failed to create auth services %s", err) os.Exit(-1) } // ... } In Go, library code (anything under internal/) should return errors and let the caller in cmd/ decide what to do. Only main() should call os.Exit or log.Fatal.

Is your feature request related to a problem? Please describe. nodeTypeIdFromTaintsAndLabels() in internal/scheduler/internaltypes/node_type.go:133 generates uint64 hashes via FNV-1a to identify node types. There is an existing TODO from the author: TODO: We should test this function to ensure there are no collisions. And that the string is never empty. Hash collisions here don't break correctness, but they reduce scheduling efficiency by grouping different node types together. A test would confirm the hash function behaves well for realistic inputs and let us remove the TODO. Describe the solution you'd like Add a test in internal/scheduler/internaltypes/node_type_test.go that: Generates a large number of realistic taint/label combinations (different taint keys, values, effects, and indexing label values) Computes the hash for each combination Asserts no two distinct inputs produce the same hash Verifies the hash input string is never empty Getting started Look at how NodeType is

Is your feature request related to a problem? Please describe. Prometheus metric label names are repeated as string literals across internal/common/metrics/scheduler_metrics.go. If someone introduces a typo (e.g., "queu" instead of "queue"), Prometheus silently creates a new metric series instead of erroring. This kind of bug is hard to catch in review and hard to debug in production. Describe the solution you'd like Define label name constants at the top of the file and use them in metric definitions instead of raw strings. File to change internal/common/metrics/scheduler_metrics.go (lines 35-175). Label names like "pool", "queue", "priorityClass", "resourceType", "nodeType", "cluster", "phase", "accounting_role", "reservation", "physical_pool", "priceBand" are each repeated across many metric definitions in this file. Example // Before (same strings repeated across many definitions) prometheus.NewDesc(MetricPrefix+"queue_resource_queued", "...", []string{"pool", "priorityClass",

Is your feature request related to a problem? Please describe. strings.Title() is deprecated since Go 1.18 and we still use it in two places in the scheduler metrics code. Describe the solution you'd like Replace both calls with cases.Title(language.English).String(...) from golang.org/x/text/cases and golang.org/x/text/language. Files to change internal/scheduler/metrics.go:484 - strings.Title(strings.ToLower(phase)) internal/scheduler/metrics.go:493 - strings.Title(strings.ToLower(phase)) Example // Before phase: strings.Title(strings.ToLower(phase)), // After phase: cases.Title(language.English).String(strings.ToLower(phase)), The cases.Title caser is safe for concurrent use, so you can create it once as a package-level variable rather than creating a new one on every call. Acceptance criteria Both strings.Title() calls replaced No strings.Title import remains go test ./internal/scheduler/... passes golangci-lint run ./internal/scheduler/... passes