Skip to content

Address bug with CSV import on sync job #314

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Address bug with CSV import on sync job #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/313.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed issue running a sync job via CSV would raise an exception.
50 changes: 28 additions & 22 deletions nautobot_device_onboarding/jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ def run(
self.memory_profiling = memory_profiling
self.debug = debug

self.job_result.task_kwargs = {
"debug": debug,
"connectivity_test": kwargs["connectivity_test"],
}

if csv_file:
self.processed_csv_data = self._process_csv_data(csv_file=csv_file)
if self.processed_csv_data:
Expand All @@ -496,10 +501,11 @@ def run(
for ip_address in self.processed_csv_data:
self.ip_addresses.append(ip_address)
# prepare the task_kwargs needed by the CommandGetterDO job
self.job_result.task_kwargs = {
"debug": debug,
"csv_file": self.task_kwargs_csv_data,
}
self.job_result.task_kwargs.update(
{
"csv_file": self.task_kwargs_csv_data,
}
)
else:
raise ValidationError(message="CSV check failed. No devices will be synced.")

Expand Down Expand Up @@ -541,24 +547,24 @@ def run(
self.secrets_group = secrets_group
self.platform = platform

self.job_result.task_kwargs = {
"debug": debug,
"location": location,
"namespace": namespace,
"ip_addresses": ip_addresses,
"set_mgmt_only": set_mgmt_only,
"update_devices_without_primary_ip": update_devices_without_primary_ip,
"device_role": device_role,
"device_status": device_status,
"interface_status": interface_status,
"ip_address_status": ip_address_status,
"port": port,
"timeout": timeout,
"secrets_group": secrets_group,
"platform": platform,
"csv_file": "",
"connectivity_test": kwargs["connectivity_test"],
}
self.job_result.task_kwargs.update(
{
"location": location,
"namespace": namespace,
"ip_addresses": ip_addresses,
"set_mgmt_only": set_mgmt_only,
"update_devices_without_primary_ip": update_devices_without_primary_ip,
"device_role": device_role,
"device_status": device_status,
"interface_status": interface_status,
"ip_address_status": ip_address_status,
"port": port,
"timeout": timeout,
"secrets_group": secrets_group,
"platform": platform,
"csv_file": "",
}
)
super().run(dryrun, memory_profiling, *args, **kwargs)


Expand Down
2 changes: 1 addition & 1 deletion nautobot_device_onboarding/nornir_plays/command_getter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def netmiko_send_commands(
return Result(
host=task.host, result=f"{task.host.name} has missing definitions in command_mapper YAML file.", failed=True
)
if orig_job_kwargs["connectivity_test"]:
if orig_job_kwargs.get("connectivity_test", False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connectivity is guaranteed to be present right? Nit pick but just curious on the .get() instead of key call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's not "guaranteed" to be present because its all getting passed in via optional kwargs from method to method. This is one of the dangers of passing around **kwargs that are optional to python but required to the logic.

one thing I could tweak is to make it a required field on the method and make the subsequent changes on upstream callers callers in this PR. OR i could add a check at the top of this method to throw an exception if its not passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the .get as a safety measure in case someone used this method in the future and didn't pass it BUT yeah i think its better to explicitly throw an exception OR require it as a method variable

if not tcp_ping(task.host.hostname, task.host.port):
return Result(
host=task.host, result=f"{task.host.name} failed connectivity check via tcp_ping.", failed=True
Expand Down
2 changes: 2 additions & 0 deletions nautobot_device_onboarding/tests/fixtures/all_required_fields.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ip_address_host,location_name,device_role_name,namespace,device_status_name,interface_status_name,ip_address_status_name,secrets_group_name,set_mgmt_only,update_devices_without_primary_ip,port,timeout
172.23.0.8,Site A Parent,Network,Global,Active,Active,Active,test secrets group,True,False,22,30
50 changes: 49 additions & 1 deletion nautobot_device_onboarding/tests/test_jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
"""Test Jobs."""

from unittest.mock import patch
from unittest.mock import ANY, patch

from django.core.files.base import ContentFile
from nautobot.apps.testing import create_job_result_and_run_job
from nautobot.core.testing import TransactionTestCase
from nautobot.dcim.models import Device, Interface, Manufacturer, Platform
from nautobot.extras.choices import JobResultStatusChoices
from nautobot.extras.models import FileProxy

from nautobot_device_onboarding import jobs
from nautobot_device_onboarding.tests import utils
Expand Down Expand Up @@ -120,6 +122,52 @@ def test_process_csv_data__empty_file(self):
processed_csv_data = onboarding_job._process_csv_data(csv_file=csv_file) # pylint: disable=protected-access
self.assertEqual(processed_csv_data, None)

@patch("nautobot_device_onboarding.diffsync.adapters.sync_devices_adapters.sync_devices_command_getter")
@patch.dict("os.environ", {"DEVICE_USER": "test_user", "DEVICE_PASS": "test_password"})
def test_csv_process_pass_connectivity_test_flag(self, mock_sync_devices_command_getter):
"""Ensure the 'connectivity_test' option is passed to the command_getter when a CSV is in-play."""
with open("nautobot_device_onboarding/tests/fixtures/all_required_fields.csv", "rb") as csv_file:
csv_contents = csv_file.read()

job_form_inputs = {
"debug": True,
"connectivity_test": "AnyWackyValueHere",
"dryrun": False,
"csv_file": FileProxy.objects.create(
name="onboarding.csv", file=ContentFile(csv_contents, name="onboarding.csv")
).id,
"location": None,
"namespace": None,
"ip_addresses": None,
"port": None,
"timeout": None,
"set_mgmt_only": None,
"update_devices_without_primary_ip": None,
"device_role": None,
"device_status": None,
"interface_status": None,
"ip_address_status": None,
"secrets_group": None,
"platform": None,
"memory_profiling": False,
}

create_job_result_and_run_job(
module="nautobot_device_onboarding.jobs", name="SSOTSyncDevices", **job_form_inputs
)
self.assertEqual(
mock_sync_devices_command_getter.mock_calls[0].args,
(
ANY,
10,
{
"debug": True,
"connectivity_test": "AnyWackyValueHere",
"csv_file": {"172.23.0.8": {"port": 22, "timeout": 30, "secrets_group": ANY, "platform": ""}},
},
),
)


class SSOTSyncNetworkDataTestCase(TransactionTestCase):
"""Test SSOTSyncNetworkData class."""
Expand Down