Skip to content

fix(scheduler): replace deprecated strings.Title with cases.Title#4783

Open
mvanhorn wants to merge 4 commits intoarmadaproject:masterfrom
mvanhorn:fix/replace-deprecated-strings-title
Open

fix(scheduler): replace deprecated strings.Title with cases.Title#4783
mvanhorn wants to merge 4 commits intoarmadaproject:masterfrom
mvanhorn:fix/replace-deprecated-strings-title

Conversation

@mvanhorn
Copy link
Copy Markdown

Summary

Replace deprecated strings.Title() (deprecated since Go 1.18) with cases.Title(language.English).String() from golang.org/x/text in the scheduler metrics code.

Changes

  • Created a package-level titleCaser variable using cases.Title(language.English) (safe for concurrent use)
  • Replaced both strings.Title() calls in internal/scheduler/metrics.go
  • Added imports for golang.org/x/text/cases and golang.org/x/text/language

Fixes #4776

This contribution was developed with AI assistance (Claude Code).

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This PR replaces the deprecated strings.Title() function (deprecated since Go 1.18) with cases.Title(language.English).String() from golang.org/x/text in the scheduler metrics code. Both call sites in updateClusterMetrics are updated inline, avoiding concurrency-safety concerns that would arise from sharing a cases.Caser instance across goroutines.

  • Replaced strings.Title() at both call sites in internal/scheduler/metrics.go (lines 500 and 509)
  • Uses cases.Title(language.English).String() directly at each call site (inlined, not a shared variable), addressing the previous reviewer concern about Caser thread safety
  • golang.org/x/text is already a declared dependency (v0.33.0), so no go.mod changes are needed
  • Minor: an extra blank line was inadvertently introduced after the import block (line 31)

Confidence Score: 5/5

Safe to merge — the fix is correct and complete, with only a trivial style nit remaining.

The deprecated strings.Title() calls are correctly replaced at both call sites. The previous concurrency concern (package-level Caser) was addressed by inlining the calls. The only remaining finding is a cosmetic extra blank line (P2), which does not affect correctness.

No files require special attention.

Important Files Changed

Filename Overview
internal/scheduler/metrics.go Replaces deprecated strings.Title() with cases.Title(language.English).String() inlined at both call sites; minor extra blank line introduced after import block

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Phase string from protobuf\n(e.g. 'JOB_RUN_STATE_RUNNING')"] --> B["strings.ToLower()\n→ 'job_run_state_running'"]
    B --> C["cases.Title(language.English).String()\n→ 'Job_Run_State_Running'"]
    C --> D["Used as Prometheus metric label\n(phase field in key struct)"]
Loading

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Reviews (5): Last reviewed commit: "Merge branch 'master' into fix/replace-d..." | Re-trigger Greptile

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mvanhorn mvanhorn force-pushed the fix/replace-deprecated-strings-title branch from 423a122 to 8039618 Compare March 19, 2026 07:25
Comment on lines +31 to +32
// titleCaser is a reusable, concurrency-safe caser for title-casing strings.
var titleCaser = cases.Title(language.English)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 cases.Caser is not concurrency-safe — the comment is incorrect

The golang.org/x/text/cases package documentation explicitly states that a Caser "may be stateful and should therefore not be shared between goroutines." Only cases.Fold is documented as stateless; cases.Title, cases.Upper, and cases.Lower are all potentially stateful.

The comment here claims the opposite (concurrency-safe), which is incorrect. Because titleCaser is a package-level variable, any concurrent calls to titleCaser.String(...) — whether from multiple test goroutines or multiple MetricsCollector instances — constitute a data race.

The simplest safe fix is to remove the package-level variable entirely and call cases.Title(language.English).String(...) directly at each call site (lines 489 and 498). That removes any sharing concern altogether.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvanhorn can you adopt this suggestion?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dd1a072. Removed the package-level titleCaser and inlined cases.Title(language.English).String() at both call sites.

mvanhorn and others added 2 commits April 1, 2026 18:56
cases.Caser is documented as potentially stateful and not safe for
concurrent use. Remove the package-level titleCaser variable and call
cases.Title(language.English).String() at each call site.

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace deprecated strings.Title() in scheduler metrics

2 participants