Return YAML contents inline from deploy API, remove /yaml endpoint#118
Return YAML contents inline from deploy API, remove /yaml endpoint#118amito wants to merge 3 commits intollm-d-incubation:mainfrom
Conversation
📝 WalkthroughWalkthroughGenerator now returns rendered YAML contents inline; the API removed the GET /api/v1/deployments/{deployment_id}/yaml endpoint and returns YAML contents in deploy responses; recommendation route, UI client, and tests updated to consume the inline Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/neuralnav/configuration/generator.py (1)
232-244:⚠️ Potential issue | 🟡 MinorDocstring is outdated.
The docstring still mentions
metadatain the return description, but the actual return now includescontentsinstead.Suggested fix
def generate_all( self, recommendation: DeploymentRecommendation, namespace: str = "default" ) -> dict[str, Any]: """ Generate all deployment YAML files. Args: recommendation: Deployment recommendation namespace: Kubernetes namespace Returns: - Dictionary with deployment_id, namespace, files, and metadata + Dictionary with deployment_id, namespace, files (paths), and contents (rendered YAML) """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/neuralnav/configuration/generator.py` around lines 232 - 244, The generate_all function's docstring return section is stale: it mentions returning `metadata` but the function now returns `contents`; update the docstring in generate_all to accurately describe the returned dict (keys like `deployment_id`, `namespace`, `files`, and `contents`) and adjust the "Returns:" text to match the current return structure and types; reference the generate_all method to locate and edit the docstring so callers/readers get correct documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/neuralnav/api/routes/recommendation.py`:
- Line 114: The assignments to result["yaml_files"] are inconsistent: in the
success path (where yaml_contents is used) result["yaml_files"] is a dict, but
in the no-viable-configs error path it’s set to an empty list; update the error
path to assign an empty dict instead of a list so the type remains consistent.
Locate the handlers in recommendation.py (the function handling the
recommendation response flow that sets result["yaml_files"]) and replace the
empty list assignment with an empty dict (or otherwise ensure both paths produce
the same dict-shaped structure).
In `@ui/components/deployment.py`:
- Around line 86-87: The deployment response handler stores inline YAML into
st.session_state.deployment_yaml_files using result.get("files") but the keys
are bare config type names (e.g., "inferenceservice") instead of proper
filenames, which makes downloaded ZIP entries unclear; update the assignment so
keys are normalized to filenames (e.g.,
f"{st.session_state.deployment_id}-inferenceservice.yaml" or append ".yaml" to
each key) before storing in st.session_state.deployment_yaml_files and ensure
the mapping transformation occurs where result.get("files") is processed
(reference st.session_state.deployment_id and the deployment_yaml_files
assignment).
---
Outside diff comments:
In `@src/neuralnav/configuration/generator.py`:
- Around line 232-244: The generate_all function's docstring return section is
stale: it mentions returning `metadata` but the function now returns `contents`;
update the docstring in generate_all to accurately describe the returned dict
(keys like `deployment_id`, `namespace`, `files`, and `contents`) and adjust the
"Returns:" text to match the current return structure and types; reference the
generate_all method to locate and edit the docstring so callers/readers get
correct documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2ef299d-c54d-4bc7-afed-c91b018001ba
📒 Files selected for processing (6)
src/neuralnav/api/routes/configuration.pysrc/neuralnav/api/routes/recommendation.pysrc/neuralnav/configuration/generator.pytests/test_yaml_generation.pyui/api_client.pyui/components/deployment.py
amito
force-pushed
the
refactor/deploy-api-response-format
branch
from
2f916a6 to
b597049
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
amito
force-pushed
the
refactor/deploy-api-response-format
branch
from
b597049 to
4828da7
Compare
amito
force-pushed
the
refactor/deploy-api-response-format
branch
from
4828da7 to
8787fc7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/neuralnav/api/routes/configuration.py (1)
83-104: Match the typed API contract to the new inline-YAML payload.Switching these handlers to
result["contents"]is the right runtime change, but the declared contract still lags behind it:DeploymentResponse.filesis stilldict[str, Any], and/deploy-to-clusterstill returns an untyped dict. Tightening those todict[str, str]and adding a response model for the cluster path would keep OpenAPI/docs/generated clients aligned with the new behavior.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 265-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/neuralnav/api/routes/configuration.py` around lines 83 - 104, The API contract still declares DeploymentResponse.files as dict[str, Any] while the handlers now return inline YAML in result["contents"] (dict[str, str]); update the DeploymentResponse model to files: dict[str, str] and adjust any other response types that return the raw dict (e.g., the /deploy-to-cluster route handler) to use a concrete response model matching the new payload; ensure the route decorator/return annotation references the updated model so OpenAPI/docs and generated clients reflect dict[str, str].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/neuralnav/api/routes/configuration.py`:
- Around line 83-104: The API contract still declares DeploymentResponse.files
as dict[str, Any] while the handlers now return inline YAML in
result["contents"] (dict[str, str]); update the DeploymentResponse model to
files: dict[str, str] and adjust any other response types that return the raw
dict (e.g., the /deploy-to-cluster route handler) to use a concrete response
model matching the new payload; ensure the route decorator/return annotation
references the updated model so OpenAPI/docs and generated clients reflect
dict[str, str].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8099020c-dbe1-48e1-a182-2d18dfd6b9ec
📒 Files selected for processing (6)
src/neuralnav/api/routes/configuration.pysrc/neuralnav/api/routes/recommendation.pysrc/neuralnav/configuration/generator.pytests/test_yaml_generation.pyui/api_client.pyui/components/deployment.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_yaml_generation.py
- src/neuralnav/api/routes/recommendation.py
- ui/components/deployment.py
- ui/api_client.py
- src/neuralnav/configuration/generator.py
Signed-off-by: rudeigerc <rudeigerc@gmail.com>
amito
force-pushed
the
refactor/deploy-api-response-format
branch
2 times, most recently
from
c3b3c30 to
cc570eb
Compare
generate_all() now returns rendered YAML strings in a 'contents' key
alongside file paths. The /deploy, /deploy-to-cluster, and /recommend
endpoints return YAML contents instead of internal file paths,
eliminating the need for a separate GET call. Removes GET
/deployments/{id}/yaml endpoint.
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Assert that the 'contents' key exists, its keys match 'files', and values are non-empty strings. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Amit Oren <amoren@redhat.com>
Remove second GET /deployments/{id}/yaml round-trip from both
api_client.deploy_and_generate_yaml() and the deployment component.
YAML contents are now read directly from the deploy response. Zip
download entries use proper filenames ({deployment_id}-{type}.yaml).
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
amito
force-pushed
the
refactor/deploy-api-response-format
branch
from
cc570eb to
c3d3d31
Compare
anfredette
left a comment
anfredette
left a comment
There was a problem hiding this comment.
Just a couple of nits for readability since this changed from files to YAML content. Otherwise, LGTM.
| "deployment_id": deployment_id, | ||
| "namespace": request.namespace, | ||
| "files": files, | ||
| "files": result["contents"], |
There was a problem hiding this comment.
| "files": result["contents"], | |
| "yaml_contents": result["contents"], |
There was a problem hiding this comment.
Can we change "files" to "yaml_contents" here and on lines 38 and 104 above?
There was a problem hiding this comment.
Also, another nit for readability, for files = result["files"] at line 238, renaming the files variable to file_paths would be a little more clear.
2 participants
Description
The deploy endpoints previously returned internal file paths in the response, requiring a second
GET /deployments/{id}/yamlround-trip to fetch the actual YAML contents. This leaked backend implementation details and added unnecessary latency for API consumers (including the MCP integration).generate_all()now captures rendered YAML strings at template render time and returns them in acontentskey. All three endpoints that call it (/deploy,/deploy-to-cluster,/recommend) return YAML content directly. TheGET /deployments/{id}/yamlendpoint has been removed.Main changes:
generator.py:generate_all()returns acontentsdict alongsidefilesconfiguration.py:/deployand/deploy-to-clusterreturncontents;GET /deployments/{id}/yamlremovedrecommendation.py:/recommendreturnscontentsinstead of file paths; error path returns{}instead of[]for type consistencyapi_client.py: Remove second GET round-tripdeployment.py: Remove second GET round-trip; zip download entries use proper filenames ({deployment_id}-{type}.yaml)How has this been tested?
test_yaml_generation.pyupdated to assertcontentskey exists, its keys matchfiles, and each value is a non-empty YAML stringuv run pytest -q)mypyandruff cleanon all changed files across all commitsSummary by CodeRabbit