-
Notifications
You must be signed in to change notification settings - Fork 675
feat: adding operator gen check to CI #4139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces validation checkpoints for the operator build pipeline. A new GitHub Actions workflow step validates uncommitted changes, the Makefile defines a check target for pre-commit validation and adds NVIDIA header insertion to RBAC files, header comments are removed from role.yaml, and API documentation formatting is updated. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/cloud/operator/Makefile (1)
116-138: Verify role.yaml header consistency with Makefile logic.The Makefile adds NVIDIA headers to all RBAC files (lines 116–138), but the AI summary indicates that
deploy/cloud/operator/config/rbac/role.yamlhas had its SPDX header comments removed. Confirm whether:
- The header removal was intentional and should be respected (and the Makefile logic adjusted), or
- The header should be restored and will be re-added by the Makefile on next generation.
This matters for CI stability—if the header is re-added by the Makefile, the
make checkvalidation could fail if the file is not committed with the new header.
🧹 Nitpick comments (1)
deploy/cloud/operator/Makefile (1)
116-138: Refactor duplicated header insertion logic.Lines 116–138 duplicate the NVIDIA header insertion pattern from lines 84–106 (CRD files). Both blocks check for the header, prepare a temporary file, and atomically replace the original. Consider extracting this into a reusable helper function or separate script to follow the DRY principle. Note: The RBAC section includes a file-existence check (
[ -f "$$file" ]) that the CRD section lacks—ensure consistency if refactored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/container-validation-backends.yml(1 hunks)deploy/cloud/operator/Makefile(2 hunks)deploy/cloud/operator/config/rbac/role.yaml(0 hunks)docs/kubernetes/api_reference.md(9 hunks)
💤 Files with no reviewable changes (1)
- deploy/cloud/operator/config/rbac/role.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T20:23:29.509Z
Learnt from: nv-hwoo
Repo: ai-dynamo/dynamo PR: 4112
File: examples/backends/sglang/deploy/agg.yaml:76-78
Timestamp: 2025-11-05T20:23:29.509Z
Learning: In DynamoGraphDeployment (DGD) YAML manifests, the volumeMounts field uses `mountPoint` (not the standard Kubernetes `mountPath`) to specify where to mount volumes. This is defined in the DGD custom resource definition API.
Applied to files:
docs/kubernetes/api_reference.md
📚 Learning: 2025-07-18T16:04:31.771Z
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
Applied to files:
deploy/cloud/operator/Makefile
🪛 checkmake (0.2.2)
deploy/cloud/operator/Makefile
[warning] 48-48: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: operator (amd64)
- GitHub Check: trtllm (amd64)
- GitHub Check: operator (arm64)
🔇 Additional comments (3)
.github/workflows/container-validation-backends.yml (1)
91-97: Good integration of pre-push validation step.The new "Check for uncommitted changes" step correctly validates that code generation steps (generate, manifests, generate-api-docs) produce consistent output before pushing the operator image. Placement after the build and before the tag/push is appropriate.
docs/kubernetes/api_reference.md (1)
80-80: Documentation formatting for proper Markdown rendering.The changes consistently escape empty braces (
{}→\{\}) in validation hint columns to prevent Markdown interpretation. This ensures the documentation renders correctly without unintended formatting. No behavioral changes to the API or documentation content—purely display formatting.Also applies to: 98-102, 279-285, 302-307, 323-325, 419-421, 440-442, 477-477
deploy/cloud/operator/Makefile (1)
47-57: Validate logic is sound, but target body exceeds style guidelines.The
checktarget correctly validates that code generation steps leave the Git tree clean—a useful validation for CI. However, static analysis (checkmake) flags that the target body (9 lines) exceeds the default threshold of 5 lines. Consider either:
- Accepting the style deviation for this validation step, or
- Refactoring into a helper shell script to reduce target size.
This is noted by the static analysis tool but is acceptable for a validation target. Confirm whether your team's conventions permit this exception.
7e71dc1 to
5cf9615
Compare
5cf9615 to
d6a2e62
Compare
c55fd79 to
d637696
Compare
Overview:
This PR adds a check to the Operator CI that checks that the output of generated code, manifests, docs match what is expected on a given commit. It will fail the workflow if it does not match.
Example of properly catching a missing license header here
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Chores
Documentation