(feat) Live Diff Workflow#116
Draft
susanhooks wants to merge 4 commits into
Draft
Conversation
Signed-off-by: Susan Hooks <shooks@nvidia.com>
📝 WalkthroughWalkthroughAdds a Temporal configuration-diff workflow, exposes it through Nautobot and the web UI, registers its API and RBAC metadata, and updates tests and UI text for the new live diff flow. ChangesConfiguration Diff Workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/nautobot/nautobot-nv-config-manager/nv_config_manager/tables.py`:
- Around line 89-93: The diff workflow is receiving the wrong managed-device
identifier because `device_id` is set from `record.device.pk` instead of the
`ConfigManagerDeviceStatus`/managed-device identifier used elsewhere. Update the
payload assembly in `tables.py` to pass `record.pk` (or the same identifier used
by the other launch points) from the `ConfigManagerDeviceStatus` context,
keeping the rest of the fields unchanged.
In
`@components/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/configmanagerdevicestatus_workflows_tab.html`:
- Around line 56-69: The workflow-tab links in
configmanagerdevicestatus_workflows_tab.html are rendered even when temporal_url
is missing, which can create broken relative URLs, and the devicename query
value is not URL-encoded, which can corrupt the link for names with reserved
characters. Update the link generation blocks around the See Diff/workflow
actions to only render when temporal_url is set, and ensure devicename is passed
through urlencoding just like site and tenant; apply the same fix to the other
workflow-tab link block noted in the template.
In `@src/nv_config_manager/temporal/ngc/workflows/config_diff.py`:
- Around line 61-63: The workflow_description string in config_diff.py is
incomplete and should be rewritten to finish the sentence cleanly. Update the
description used by the config diff workflow so it reads naturally with a
complete object after “without applying” (for example, by adding “it” or
“changes”), keeping the wording user-facing because this value is surfaced in
the UI and exported into the Temporal API docs. Locate the text in
workflow_description and adjust only the description content, not the
surrounding workflow logic.
In `@ui/src/app/workflows/configdiffworkflow/form/page.tsx`:
- Around line 30-41: `DeviceWorkflowForm` leaves `isSubmitting` stuck because
`onSubmit` does not await or return the `startWorkflow` promise and never clears
the submitting state. Update the submit handler in `DeviceWorkflowForm` to be
async, await `startWorkflow(endpoint, params)`, keep the existing toast handling
on failure, and reset `isSubmitting` in a `finally` block so the form can be
retried after success or error.
In `@ui/src/components/forms/workflow.tsx`:
- Around line 269-292: The manual-change guards in the workflow form are never
cleared, so URL-driven tenant/status updates can be blocked after a user change.
Update the React.useEffect logic in workflow.tsx around the tenant and status
preselect blocks to reset the corresponding isManualTenantChange and
isManualStatusChange flags whenever queryTenants or queryStatuses changes, or
track a query signature so new URL selections can re-apply. Keep the fix
localized to the effects that call form.setValue for tenant and status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3cc862e8-7b8c-4bb3-b738-959d0c188958
📒 Files selected for processing (18)
components/nautobot/nautobot-nv-config-manager/nv_config_manager/tables.pycomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/configmanagerdevicestatus_workflows_tab.htmlcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/inc/managed_device_details.htmlcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/inc/see_diff.htmlcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/tests/test_views.pycomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/views.pydeploy/helm/values-rbac-open.yamldocs/api-specs/temporal.openapi.jsonsrc/nv_config_manager/temporal/ngc/workflows/__init__.pysrc/nv_config_manager/temporal/ngc/workflows/config_diff.pysrc/tests/temporal/ngc/workflows/test_config_diff_workflow.pyui/src/app/device/[uuid]/[filename]/history/page.tsxui/src/app/workflows/configdiffworkflow/form/loading.tsxui/src/app/workflows/configdiffworkflow/form/page.tsxui/src/components/forms/workflow.tsxui/src/config/site.tsui/src/types/data-table.types.tsui/tsconfig.tsbuildinfo
Comment on lines
+89
to
+93
| "temporal_url": settings.PLUGINS_CONFIG["nv_config_manager"].get("temporal_url"), | ||
| "site": site, | ||
| "device_id": record.device.pk, | ||
| "tenant": getattr(record.device.tenant, "name", "") if record else "", | ||
| "status": getattr(record.device.status, "name", "") if record else "", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass the managed-device identifier to the diff workflow.
device_id is populated with record.device.pk, but the other launch points use ConfigManagerDeviceStatus.pk (instance.pk or managed_device.pk). This can cause the workflow to compare the wrong object or fail lookup. Use record.pk (or the workflow’s documented identifier) consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/nautobot/nautobot-nv-config-manager/nv_config_manager/tables.py`
around lines 89 - 93, The diff workflow is receiving the wrong managed-device
identifier because `device_id` is set from `record.device.pk` instead of the
`ConfigManagerDeviceStatus`/managed-device identifier used elsewhere. Update the
payload assembly in `tables.py` to pass `record.pk` (or the same identifier used
by the other launch points) from the `ConfigManagerDeviceStatus` context,
keeping the rest of the fields unchanged.
Comment on lines
+30
to
+41
| const onSubmit = (data: DeviceWorkflowFormSchema) => { | ||
| const endpoint = "/v1/workflow/ngc/config_diff"; | ||
| const params: ConfigDiffWorkflowInput = { | ||
| device_id: data.device, | ||
| }; | ||
| startWorkflow(endpoint, params).catch((error) => { | ||
| toast({ | ||
| variant: "destructive", | ||
| title: "Workflow Failed", | ||
| description: `Failed to create workflow: ${error}`, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset submitting state after the workflow request completes.
DeviceWorkflowForm sets isSubmitting to true, but this callback never returns the startWorkflow promise or resets the state. On rejection the toast is shown while the form remains permanently disabled, preventing retry; on success it also remains stuck if no navigation occurs. Make the submit contract async and clear isSubmitting in a finally block in DeviceWorkflowForm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/src/app/workflows/configdiffworkflow/form/page.tsx` around lines 30 - 41,
`DeviceWorkflowForm` leaves `isSubmitting` stuck because `onSubmit` does not
await or return the `startWorkflow` promise and never clears the submitting
state. Update the submit handler in `DeviceWorkflowForm` to be async, await
`startWorkflow(endpoint, params)`, keep the existing toast handling on failure,
and reset `isSubmitting` in a `finally` block so the form can be retried after
success or error.
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Collaborator
Author
|
/ok to test 4cbd7c8 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nv_config_manager/temporal/ngc/workflows/config_diff.py (1)
165-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEscape or use a collision-safe Markdown fence for device-generated diffs.
A configuration value can contain triple backticks, which would terminate this fence early and render the remainder as Markdown instead of code. Use a longer/generated fence or escape the content before displaying it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nv_config_manager/temporal/ngc/workflows/config_diff.py` around lines 165 - 166, The markdown rendering in the diff output is vulnerable to fence collisions because the device-generated diff can contain triple backticks and break the surrounding code block. Update the logic in the PerformDiffStageOutput construction path in config_diff.py to use a collision-safe fence length or otherwise escape/sanitize the diff content before embedding it in the markdown string. Keep the display field safe for arbitrary configuration values while preserving the diff text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/nv_config_manager/temporal/ngc/workflows/config_diff.py`:
- Around line 165-166: The markdown rendering in the diff output is vulnerable
to fence collisions because the device-generated diff can contain triple
backticks and break the surrounding code block. Update the logic in the
PerformDiffStageOutput construction path in config_diff.py to use a
collision-safe fence length or otherwise escape/sanitize the diff content before
embedding it in the markdown string. Keep the display field safe for arbitrary
configuration values while preserving the diff text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23e80bf7-b0e4-43d6-90f6-ad7aeef71ae2
📒 Files selected for processing (7)
components/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/configmanagerdevicestatus_workflows_tab.htmlcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/inc/see_diff.htmlcomponents/nautobot/nautobot-nv-config-manager/nv_config_manager/tests/test_views.pysrc/nv_config_manager/temporal/ngc/workflows/config_diff.pysrc/tests/temporal/ngc/workflows/test_config_diff_workflow.pyui/src/app/workflows/configdiffworkflow/form/page.tsxui/tsconfig.tsbuildinfo
💤 Files with no reviewable changes (1)
- components/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/inc/see_diff.html
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/src/app/workflows/configdiffworkflow/form/page.tsx
- components/nautobot/nautobot-nv-config-manager/nv_config_manager/templates/nv_config_manager/configmanagerdevicestatus_workflows_tab.html
- components/nautobot/nautobot-nv-config-manager/nv_config_manager/tests/test_views.py
- src/tests/temporal/ngc/workflows/test_config_diff_workflow.py
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
1 participant
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
Validation
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for
review, approve the current commit and start the suite with these PR comments:
As a fallback, run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite, or narrow itonly while debugging.
The completed workflow posts its conclusion and exact run URL as a PR comment.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit