fix(budcluster): enforce 63 char limit for Kubernetes namespace names #1102
Conversation
- Fix _get_namespace() to truncate endpoint names to 50 chars max - Add consecutive hyphen removal and cleanup in namespace generation - Add max_length=50 validation to benchmark name schemas (budapp, budcluster) - Add frontend validation with maxLength prop and error toast display - Prevents Ansible failures when deploying benchmarks with long names 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @dittops, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where long benchmark names could lead to Kubernetes namespace names exceeding the 63-character limit, causing deployment failures. The solution involves a comprehensive, multi-layered approach to enforce a 50-character limit for benchmark names, ensuring that the generated Kubernetes namespaces remain within acceptable bounds. Additionally, the changes improve the user experience by providing clearer feedback through enhanced error handling for API validation failures. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively enforces the 63-character limit for Kubernetes namespaces by adding validation on multiple layers (frontend, backend schemas) and implementing robust truncation logic on the backend. The improved error handling on the frontend to display API validation errors is also a great addition.
I've provided two suggestions:
- A high-severity fix in
deployment/handler.pyto handle edge cases in namespace generation that could lead to invalid names. - A medium-severity refactoring suggestion in
usePerfomanceBenchmark.tsxto improve the clarity and maintainability of the new error handling logic.
Overall, this is a solid and well-thought-out fix.
| if len(cleaned_name) > max_name_length: | ||
| cleaned_name = cleaned_name[:max_name_length].rstrip("-") | ||
| return f"bud-{cleaned_name}-{unique_id}" |
There was a problem hiding this comment.
The current logic has two edge cases that can produce invalid Kubernetes namespaces:
- Names shorter than 50 chars but ending with a hyphen are not handled, leading to invalid names like
bud-my-name--...because therstripis only inside theifblock for long names. - Empty names after cleaning (e.g., from input
_) result in invalid names likebud--....
The suggested change makes the truncation and cleaning more robust to handle these cases by always stripping trailing hyphens and providing a fallback for empty names.
| if len(cleaned_name) > max_name_length: | |
| cleaned_name = cleaned_name[:max_name_length].rstrip("-") | |
| return f"bud-{cleaned_name}-{unique_id}" | |
| cleaned_name = cleaned_name[:max_name_length].rstrip("-") | |
| if not cleaned_name: | |
| cleaned_name = "benchmark" | |
| return f"bud-{cleaned_name}-{unique_id}" |
| // Handle validation errors from API (Pydantic 422 errors) | ||
| if (error?.response?.data?.detail) { | ||
| const details = error.response.data.detail; | ||
| if (Array.isArray(details)) { | ||
| const messages = details.map((d: any) => d.msg || d.message).join(", "); | ||
| errorToast(messages || "Validation error"); | ||
| } else if (typeof details === "string") { | ||
| errorToast(details); | ||
| } else { | ||
| errorToast("Validation error"); | ||
| } | ||
| } else if (error?.response?.data?.message) { | ||
| errorToast(error.response.data.message); | ||
| } else if (error?.message) { | ||
| errorToast(error.message); | ||
| } else { | ||
| errorToast("Failed to create benchmark"); | ||
| } |
There was a problem hiding this comment.
The error handling logic is a bit verbose with multiple errorToast calls inside the if/else chain. This can be refactored to be more concise and maintainable by first determining the error message and then calling errorToast once at the end.
| // Handle validation errors from API (Pydantic 422 errors) | |
| if (error?.response?.data?.detail) { | |
| const details = error.response.data.detail; | |
| if (Array.isArray(details)) { | |
| const messages = details.map((d: any) => d.msg || d.message).join(", "); | |
| errorToast(messages || "Validation error"); | |
| } else if (typeof details === "string") { | |
| errorToast(details); | |
| } else { | |
| errorToast("Validation error"); | |
| } | |
| } else if (error?.response?.data?.message) { | |
| errorToast(error.response.data.message); | |
| } else if (error?.message) { | |
| errorToast(error.message); | |
| } else { | |
| errorToast("Failed to create benchmark"); | |
| } | |
| // Handle validation errors from API (Pydantic 422 errors) | |
| let errorMessage = "Failed to create benchmark"; | |
| const detail = error?.response?.data?.detail; | |
| if (detail) { | |
| if (Array.isArray(detail)) { | |
| errorMessage = detail.map((d: any) => d.msg || d.message).join(", ") || "Validation error"; | |
| } else if (typeof detail === "string") { | |
| errorMessage = detail; | |
| } else { | |
| errorMessage = "Validation error"; | |
| } | |
| } else if (error?.response?.data?.message) { | |
| errorMessage = error.response.data.message; | |
| } else if (error?.message) { | |
| errorMessage = error.message; | |
| } | |
| errorToast(errorMessage); |
- Always truncate and strip trailing hyphens from namespace names - Add fallback to "benchmark" for empty names after cleaning - Refactor error handling to be more concise with single errorToast call - Fix ruff formatting: add blank lines before module-level constants 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Changes
Backend (budcluster)
deployment/handler.py: Fixed_get_namespace()to truncate endpoint names to 50 chars max, remove consecutive hyphens, and ensure namespace stays within 63 char limitbenchmark_ops/schemas.py: Addedmax_length=50validation to benchmark name fieldBackend (budapp)
benchmark_ops/schemas.py: Addedmax_length=50validation to benchmark name fieldFrontend (budadmin)
AddBenchmark.tsx: AddedmaxLength={50}prop and validation rule for benchmark name inputTextInput.tsx: AddedmaxLengthprop support to the componentusePerfomanceBenchmark.tsx: Added proper error handling to display API validation errors as toast notificationsTest plan
🤖 Generated with Claude Code