Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new test target and test module for workflow spec parsing, updates parse_workflow_spec to use a unified section parser for "workflow" and "default-values", and preserves the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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
🧹 Nitpick comments (2)
src/lib/utils/tests/test_workflow.py (1)
75-84: Consider adding test for duplicate non-workflow keys.The test verifies duplicate
workflowkeys raise an error. Consider adding a test for duplicatedefault-valuessections to ensure the duplicate detection is key-agnostic.🧪 Additional test suggestion
def test_duplicate_default_values_raises(self): spec = """\ default-values: foo: bar default-values: baz: qux """ with self.assertRaises(osmo_errors.OSMOUserError) as context: workflow_utils.parse_workflow_spec(spec) self.assertIn('default-values', str(context.exception))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/tests/test_workflow.py` around lines 75 - 84, Add a new unit test to cover duplicate non-workflow keys by creating a test method similar to test_duplicate_workflow_raises named test_duplicate_default_values_raises that passes a YAML string with two duplicate default-values sections into workflow_utils.parse_workflow_spec and asserts it raises osmo_errors.OSMOUserError and that the exception message contains "default-values"; reference the existing pattern in test_duplicate_workflow_raises (use parse_workflow_spec and osmo_errors.OSMOUserError) to mirror setup and assertions so duplicate detection is verified for keys other than "workflow".src/utils/job/app.py (1)
294-298: Consider consistent unknown section validation.This function validates
workflowsection presence but doesn't check for unknown top-level keys, whileTemplateSpec.load_template_with_variablesinsrc/utils/job/workflow.pyraises an error for unknown sections. If this difference is intentional (e.g., apps can have additional sections), the current code is fine. Otherwise, consider aligning the validation logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/job/app.py` around lines 294 - 298, validate_app_content currently only checks for presence of the 'workflow' section (using workflow_utils.parse_workflow_spec) but does not validate unknown top-level keys like TemplateSpec.load_template_with_variables does; decide whether apps should allow extra sections—if not, update validate_app_content to compute the set difference between parsed sections and the allowed set (e.g., {'workflow'}) and raise an osmo_errors.OSMOUserError listing unknown sections, otherwise add a comment explaining the intentional difference and/or mirror the same allowed-sections logic used by TemplateSpec.load_template_with_variables for consistency.
🤖 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/lib/utils/workflow.py`:
- Around line 40-61: The docstring for parse_workflow_spec incorrectly states it
raises OSMOUserError when no "workflow" section is found; update the docstring
to accurately describe behavior: say the function parses top-level sections into
a mapping of names to raw text and only raises OSMOUserError on duplicate
top-level keys (it does not validate presence of a specific "workflow" section),
and note that callers are responsible for ensuring required sections exist;
reference the parse_workflow_spec function and the duplicate-key behavior in the
updated description.
In `@src/utils/job/workflow.py`:
- Around line 788-799: The code currently sets template_data =
yaml.safe_load(sections['default-values'])['default-values'] which can produce
None when the YAML block exists but is empty; update the logic in workflow
parsing (around parse_workflow_spec usage and the template_data variable) to
safely handle that case by loading the YAML, extracting the 'default-values' key
via .get(...) or intermediate variable, and if the result is None set
template_data = {} (or cast to dict) so downstream code always receives a dict;
ensure you modify only the block that reads sections['default-values'] and
preserve existing behavior when a real mapping is present.
---
Nitpick comments:
In `@src/lib/utils/tests/test_workflow.py`:
- Around line 75-84: Add a new unit test to cover duplicate non-workflow keys by
creating a test method similar to test_duplicate_workflow_raises named
test_duplicate_default_values_raises that passes a YAML string with two
duplicate default-values sections into workflow_utils.parse_workflow_spec and
asserts it raises osmo_errors.OSMOUserError and that the exception message
contains "default-values"; reference the existing pattern in
test_duplicate_workflow_raises (use parse_workflow_spec and
osmo_errors.OSMOUserError) to mirror setup and assertions so duplicate detection
is verified for keys other than "workflow".
In `@src/utils/job/app.py`:
- Around line 294-298: validate_app_content currently only checks for presence
of the 'workflow' section (using workflow_utils.parse_workflow_spec) but does
not validate unknown top-level keys like
TemplateSpec.load_template_with_variables does; decide whether apps should allow
extra sections—if not, update validate_app_content to compute the set difference
between parsed sections and the allowed set (e.g., {'workflow'}) and raise an
osmo_errors.OSMOUserError listing unknown sections, otherwise add a comment
explaining the intentional difference and/or mirror the same allowed-sections
logic used by TemplateSpec.load_template_with_variables for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0aab3760-6caa-4859-9f3d-4da62e522b9b
📒 Files selected for processing (7)
src/cli/workflow.pysrc/lib/utils/tests/BUILDsrc/lib/utils/tests/test_workflow.pysrc/lib/utils/workflow.pysrc/service/core/workflow/objects.pysrc/utils/job/app.pysrc/utils/job/workflow.py
💤 Files with no reviewable changes (1)
- src/service/core/workflow/objects.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/utils/workflow.py (1)
20-20: Consider using built-indictfor type hints (Python 3.9+ style).Since Python 3.9+, you can use
dict[str, str]directly without importingDictfromtyping.♻️ Proposed fix
-from typing import Dict +# No typing import needed if using dict directlyThen update line 40 and 53:
-def parse_workflow_spec(workflow_spec: str) -> Dict[str, str]: +def parse_workflow_spec(workflow_spec: str) -> dict[str, str]:- sections: Dict[str, str] = {} + sections: dict[str, str] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/workflow.py` at line 20, Remove the typing.Dict import and use built-in generic dict annotations instead: replace the import "from typing import Dict" with no import, and change any type hints that use Dict (those at the occurrences around line 40 and line 53) to the modern syntax dict[str, str] (or the appropriate key/value types used there) so annotations use native dict generics.
🤖 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/lib/utils/workflow.py`:
- Line 20: Remove the typing.Dict import and use built-in generic dict
annotations instead: replace the import "from typing import Dict" with no
import, and change any type hints that use Dict (those at the occurrences around
line 40 and line 53) to the modern syntax dict[str, str] (or the appropriate
key/value types used there) so annotations use native dict generics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb4bdd6d-f47a-4dd1-b74f-d1e2ad2a4ba1
📒 Files selected for processing (2)
src/lib/utils/tests/test_workflow.pysrc/lib/utils/workflow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/utils/tests/test_workflow.py
ecolternv
force-pushed
the
ecolter/resource-parse-fix
branch
from
e5ce3d6 to
89143a2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/utils/tests/test_workflow.py (2)
60-71: Consider clarifying the test name.The test name
test_jinja_content_not_at_root_indentis potentially misleading since the Jinja directives ({% for %},{% endfor %}) ARE at root indent (column 0). The test appears to verify that Jinja syntax at column 0 doesn't break workflow extraction because it's not recognized as a YAML key.A clearer name might be
test_jinja_directives_at_column_zeroortest_jinja_content_does_not_terminate_workflow_capture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/tests/test_workflow.py` around lines 60 - 71, Rename the test function test_jinja_content_not_at_root_indent to a clearer name (e.g., test_jinja_directives_at_column_zero or test_jinja_content_does_not_terminate_workflow_capture) in the test class so its intent matches the assertion verifying workflow_utils.parse_workflow_spec handles Jinja directives at column 0; update the test function name wherever referenced (imports/pytest markers) to keep tests runnable.
73-82: Consider adding tests for other error conditions.The test suite covers the duplicate workflow error case well. However, looking at
parse_workflow_specinsrc/lib/utils/workflow.py, there are two additional error conditions that aren't tested:
- Multiple
default-valuessections raisesOSMOUserError- Missing workflow spec (no
workflow:key found) raisesOSMOUserErrorAdding tests for these would improve coverage of the error handling paths.
🧪 Suggested additional test cases
def test_duplicate_default_values_raises(self): spec = """\ default-values: foo: bar default-values: baz: qux workflow: name: my-wf """ with self.assertRaises(osmo_errors.OSMOUserError) as context: workflow_utils.parse_workflow_spec(spec) self.assertIn('default-values', str(context.exception)) def test_missing_workflow_raises(self): spec = """\ default-values: foo: bar """ with self.assertRaises(osmo_errors.OSMOUserError) as context: workflow_utils.parse_workflow_spec(spec) self.assertIn('Workflow spec not found', str(context.exception))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/tests/test_workflow.py` around lines 73 - 82, Add two new unit tests alongside test_duplicate_workflow_raises to cover the other error paths in parse_workflow_spec: (1) test_duplicate_default_values_raises — feed a spec with two "default-values:" sections and assert parse_workflow_spec raises osmo_errors.OSMOUserError and the exception string contains "default-values"; (2) test_missing_workflow_raises — feed a spec with only "default-values:" and assert parse_workflow_spec raises osmo_errors.OSMOUserError and the exception string contains "Workflow spec not found". Place the tests in src/lib/utils/tests/test_workflow.py near test_duplicate_workflow_raises and reference parse_workflow_spec and osmo_errors.OSMOUserError in the assertions.
🤖 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/lib/utils/tests/test_workflow.py`:
- Around line 60-71: Rename the test function
test_jinja_content_not_at_root_indent to a clearer name (e.g.,
test_jinja_directives_at_column_zero or
test_jinja_content_does_not_terminate_workflow_capture) in the test class so its
intent matches the assertion verifying workflow_utils.parse_workflow_spec
handles Jinja directives at column 0; update the test function name wherever
referenced (imports/pytest markers) to keep tests runnable.
- Around line 73-82: Add two new unit tests alongside
test_duplicate_workflow_raises to cover the other error paths in
parse_workflow_spec: (1) test_duplicate_default_values_raises — feed a spec with
two "default-values:" sections and assert parse_workflow_spec raises
osmo_errors.OSMOUserError and the exception string contains "default-values";
(2) test_missing_workflow_raises — feed a spec with only "default-values:" and
assert parse_workflow_spec raises osmo_errors.OSMOUserError and the exception
string contains "Workflow spec not found". Place the tests in
src/lib/utils/tests/test_workflow.py near test_duplicate_workflow_raises and
reference parse_workflow_spec and osmo_errors.OSMOUserError in the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a9ba240-6e3e-4fc4-bf4f-4d0776d3c85f
📒 Files selected for processing (1)
src/lib/utils/tests/test_workflow.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/utils/workflow.py (2)
64-65: Uncaught YAML parsing errors could expose internal exceptions to users.If the
default-valuessection contains malformed YAML,yaml.safe_load()will raise ayaml.YAMLErrorthat propagates uncaught. Consider wrapping this in a try-except to provide a user-friendly error message consistent with other validation errors in this function.♻️ Proposed fix to wrap YAML errors
default_values = None if 'default-values' in sections: - default_values = yaml.safe_load(sections['default-values'])['default-values'] + try: + default_values = yaml.safe_load(sections['default-values'])['default-values'] + except yaml.YAMLError as e: + raise osmo_errors.OSMOUserError( + f'Invalid YAML in default-values section: {e}') from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/workflow.py` around lines 64 - 65, The YAML parse of the 'default-values' section can raise yaml.YAMLError and currently propagates uncaught; wrap the yaml.safe_load(sections['default-values']) call in a try-except that catches yaml.YAMLError and re-raises a user-friendly validation error (e.g., ValueError or the same ValidationError type used elsewhere in this function) with a clear message referencing 'default-values' so callers see a consistent, non-internal error; update the assignment to default_values only after successful parsing.
42-43: Consider expanding the docstring to document the return value.The docstring is minimal. It would help future maintainers to document what the tuple contains and what errors the function raises.
📝 Proposed docstring improvement
def parse_workflow_spec(workflow_spec: str) -> Tuple[str, Dict | None]: - """ Parse the workflow spec. """ + """ + Parse the workflow spec into workflow content and default values. + + Returns: + A tuple of (workflow_section_text, default_values_dict). + default_values_dict is None if no default-values section is present. + + Raises: + OSMOUserError: If an unknown top-level key is found, a duplicate key exists, + or no workflow section is present. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/workflow.py` around lines 42 - 43, Update the docstring for parse_workflow_spec to explicitly describe the return tuple (first element: workflow name as str; second element: parameters as Dict or None), the possible shapes of the Dict (e.g., mapping of parameter names to values), and the exceptions the function may raise (e.g., ValueError, JSONDecodeError or other parsing errors thrown by the implementation); keep it brief and follow the project's docstring style (one-line summary, longer description, Returns, Raises) so callers and maintainers know what to expect from parse_workflow_spec.
🤖 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/lib/utils/workflow.py`:
- Around line 64-65: The YAML parse of the 'default-values' section can raise
yaml.YAMLError and currently propagates uncaught; wrap the
yaml.safe_load(sections['default-values']) call in a try-except that catches
yaml.YAMLError and re-raises a user-friendly validation error (e.g., ValueError
or the same ValidationError type used elsewhere in this function) with a clear
message referencing 'default-values' so callers see a consistent, non-internal
error; update the assignment to default_values only after successful parsing.
- Around line 42-43: Update the docstring for parse_workflow_spec to explicitly
describe the return tuple (first element: workflow name as str; second element:
parameters as Dict or None), the possible shapes of the Dict (e.g., mapping of
parameter names to values), and the exceptions the function may raise (e.g.,
ValueError, JSONDecodeError or other parsing errors thrown by the
implementation); keep it brief and follow the project's docstring style
(one-line summary, longer description, Returns, Raises) so callers and
maintainers know what to expect from parse_workflow_spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90922ef4-c9fa-4202-9f8a-ed3aac4cb77d
📒 Files selected for processing (1)
src/lib/utils/workflow.py
ecolternv
force-pushed
the
ecolter/resource-parse-fix
branch
from
aedaf8e to
aa08dfd
Compare
fernandol-nvidia
approved these changes
Mar 13, 2026
cypres
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix resource parsing
Issue - None
Checklist
Summary by CodeRabbit
Tests
Bug Fixes
New Features