Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR modifies the workflow job node prompt data merging logic to filter out empty-string values from workflow job prompts before updating node prompts. It includes a functional test that verifies a workflow job node's limit is not overridden by an empty-string workflow job limit. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
awx/main/tests/functional/models/test_workflow.py (1)
304-309: Assert the normalization contract directly in this test.This test proves downstream behavior, but it should also verify that
wj.limit = ''removeslimitfromchar_prompts(root contract). The current inline comment also reflects pre-fix behavior.Proposed test tightening
- # This stores '' in char_prompts instead of treating it as None/"no limit". + # Empty string should be normalized to "unset" (removed from char_prompts). wj = WorkflowJob.objects.create(name='test-wf-job') - wj.limit = '' # stores {'limit': ''} in char_prompts - the IaC bug scenario + wj.limit = '' wj.save() + assert 'limit' not in wj.char_promptsAlso applies to: 315-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/functional/models/test_workflow.py` around lines 304 - 309, Add explicit assertions in this test to verify the normalization contract: after creating wj = WorkflowJob(...), setting wj.limit = '' and calling wj.save() assert that wj.char_prompts does not contain the 'limit' key (or that its value is None/removed), and repeat the same explicit assertion for the similar case around lines 315-319; locate the logic via the WorkflowJob model, the limit attribute mutation, and the char_prompts field to implement the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@awx/main/tests/functional/models/test_workflow.py`:
- Around line 304-309: Add explicit assertions in this test to verify the
normalization contract: after creating wj = WorkflowJob(...), setting wj.limit =
'' and calling wj.save() assert that wj.char_prompts does not contain the
'limit' key (or that its value is None/removed), and repeat the same explicit
assertion for the similar case around lines 315-319; locate the logic via the
WorkflowJob model, the limit attribute mutation, and the char_prompts field to
implement the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc19cc82-4805-4473-bae0-82693e5db7d3
📒 Files selected for processing (2)
awx/main/tests/functional/models/test_workflow.pyawx/main/utils/common.py
| node_prompts_data.update(wj_prompts_data) | ||
| # Empty string values on the workflow job (e.g. from IaC setting limit: "") | ||
| # should not override a node's explicit non-empty prompt value | ||
| node_prompts_data.update({k: v for k, v in wj_prompts_data.items() if v != ''}) |
There was a problem hiding this comment.
The workflow job prompts actually do distinguish not-set versus "". Empty string indicates "target all hosts". And the UI should have this implemented correctly on its side.
There was a problem hiding this comment.
@AlanCoding so the current behavior is actually the correct one?
4 participants
SUMMARY
Treat an empty string as
no value setfor prompted fields and don't store the empty string value. This allows node level fields in a workflow to override them.ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
Summary by CodeRabbit