diff --git a/docs/user/design_lifecycle.md b/docs/user/design_lifecycle.md index 871ab57..27a6149 100644 --- a/docs/user/design_lifecycle.md +++ b/docs/user/design_lifecycle.md @@ -91,24 +91,22 @@ The decommissioning job outputs a log with all the detailed operation reverting ### Design Deployment Import -Design Builder helps in greenfield use cases (i.e., creating a new data from a design) and in brownfield ones too (i.e., importing existing data that are are related to a new deployment). In the "deployment" mode, a Design Deployment tracks all the objects and attributes that are "owned" by it. With the import functionality, orphan objects and attributes will be incorporated to a new Design Deployment as if they have been set by it. +Design Builder addresses -The import logic works like this: +- greenfield use cases by creating new data from a design +- brownfield use cases by importing existing data related to a new design deployment -1. If the object that we reference doesn't exist, normal design creation logic applies. +In the "deployment" mode, a design deployment tracks all the objects and attributes that are "owned" by it. With the import functionality, orphan objects and attributes will be incorporated into a new design deployment as if they have been set by it. -2. If an object that we want to "create" already exists - 2.1. If it's not owned by another design deployment, we get "full_control" of it, and of all the attributes that we define (including the identifiers). - 2.2. If it's owned, the process fails with an exception because the intention of "create" is to have ownership. +The import logic works like this: +1. If the object that we reference doesn't exist, normal design creation logic applies +2. If an object that we want to "create" already exists, normal design creation logic _also_ applies 3. If an object that we want to "create_or_update" already exists - 3.1. If it's not owned by another design deployment, we get "full_control" of it, and of all the attributes that we define (including the identifiers). - 3.2. If it has already an owner, we don't claim ownership of the object, but we still may claim the attributes, except the identifiers. - + - If it's not owned by another design deployment, we get "full_control" of it and of all the attributes that we define (including the identifiers) + - If it already has an owner, we don't claim ownership of the object, but we still may claim the attributes, except the identifiers 4. If an object that we want to "update" already exists - 3.1. There is no claim for full_control ownership. - 3.2. There is claim for the attributes, except the identifiers. - + - There is no claim for "full_control" ownership + - There is a claim for the attributes, except the identifiers 5. In all cases, the attributes that a design is trying to update are claimed. These attributes can't be claimed by any other design. If so, the import fails pointing to the conflict dependency. - -6. The imported changes (attributes) show the same old and new value because we can't infer which was the previous value (in most cases, it would be `null` but we can't be sure). +6. The imported changes (attributes) show the same old and new value because we can't infer which was the previous value (in most cases, it would be `null` but we can't be sure) diff --git a/nautobot_design_builder/design.py b/nautobot_design_builder/design.py index ac7d776..07e3e09 100644 --- a/nautobot_design_builder/design.py +++ b/nautobot_design_builder/design.py @@ -62,7 +62,6 @@ def log(self, model: "ModelInstance"): if self.change_set: self.change_set.log(model, self.import_mode) - # TODO: CA - I don't understand this if instance.pk not in self.index: self.index.add(instance.pk) @@ -130,6 +129,8 @@ class ModelMetadata: # pylint: disable=too-many-instance-attributes CREATE_OR_UPDATE = "create_or_update" ACTION_CHOICES = [GET, CREATE, UPDATE, CREATE_OR_UPDATE] + # Actions that work with import mode + IMPORTABLE_ACTION_CHOICES = [UPDATE, CREATE_OR_UPDATE] def __init__(self, model_instance: "ModelInstance", **kwargs): """Initialize the metadata object for a given model instance. @@ -537,9 +538,7 @@ def _load_instance(self): # pylint: disable=too-many-branches self.instance = self.model_class.objects.get(**query_filter) return - if self.metadata.action in [ModelMetadata.UPDATE, ModelMetadata.CREATE_OR_UPDATE] or ( - self.metadata.action is ModelMetadata.CREATE and self.environment.import_mode - ): + if self.metadata.action in [ModelMetadata.UPDATE, ModelMetadata.CREATE_OR_UPDATE]: # perform nested lookups. First collect all the # query params for top-level relationships, then # perform the actual lookup @@ -566,9 +565,6 @@ def _load_instance(self): # pylint: disable=too-many-branches field_values[query_param] = model try: self.instance = self.relationship_manager.get(**query_filter) - if self.environment.import_mode and self.metadata.action != ModelMetadata.UPDATE: - self.metadata.attributes.update(field_values) - return except ObjectDoesNotExist: if self.metadata.action == ModelMetadata.UPDATE: @@ -782,7 +778,7 @@ def implement_design(self, design: Dict, commit: bool = False): try: for key, value in design.items(): if key in self.model_map and value: - self._create_or_import_objects(self.model_map[key], value) + self._create_objects(self.model_map[key], value) elif key not in self.model_map: raise errors.DesignImplementationError(f"Unknown model key {key} in design") @@ -852,7 +848,8 @@ def resolve_values(self, value: Union[list, dict, str]) -> Any: value[k] = self.resolve_value(item) return value - def _create_or_import_objects(self, model_class: Type[ModelInstance], objects: Union[List[Any], Dict[str, Any]]): + # TODO(2.x): Rename to `_create_or_import-objects` + def _create_objects(self, model_class: Type[ModelInstance], objects: Union[List[Any], Dict[str, Any]]): if isinstance(objects, dict): model = model_class(self, objects) model.save() diff --git a/nautobot_design_builder/design_job.py b/nautobot_design_builder/design_job.py index eedc947..eebbdd8 100644 --- a/nautobot_design_builder/design_job.py +++ b/nautobot_design_builder/design_job.py @@ -81,18 +81,6 @@ def determine_deployment_name(cls, data): return data["deployment_name"] return data[deployment_name_field] - @classmethod - def determine_import_mode(cls, data): - """Determine the import mode, if specified.""" - if not cls.is_deployment_job(): - return None - - if "import_mode" not in data: - return False - # TODO: not sure why not got the default to False from _get_vars - # raise DesignImplementationError("No import mode was provided for the deployment.") - return data["import_mode"] - @classmethod def _get_vars(cls): """Retrieve the script variables for the job. @@ -294,7 +282,7 @@ def _run_in_transaction(self, **kwargs): # pylint: disable=too-many-branches, t self.job_result.job_kwargs = {"data": self.serialize_data(data)} - data["import_mode"] = self.determine_import_mode(data) + data["import_mode"] = self.is_deployment_job() and data.get("import_mode", False) data["deployment_name"] = self.determine_deployment_name(data) change_set, previous_change_set = self._setup_changeset(data["deployment_name"]) self.log_info(message=f"Building {getattr(self.Meta, 'name')}") @@ -306,7 +294,7 @@ def _run_in_transaction(self, **kwargs): # pylint: disable=too-many-branches, t import_mode=data["import_mode"], ) - if data["import_mode"] and data["deployment_name"]: + if data["import_mode"]: self.log_info(message=f'Running in import mode for {data["deployment_name"]}.') design_files = None diff --git a/nautobot_design_builder/jobs.py b/nautobot_design_builder/jobs.py index a847108..33de3ae 100644 --- a/nautobot_design_builder/jobs.py +++ b/nautobot_design_builder/jobs.py @@ -17,9 +17,9 @@ class DeploymentDecommissioning(Job): query_params={"status": "active"}, description="Design Deployments to decommission.", ) - only_traceability = BooleanVar( - description="Only remove the objects traceability, not decommissioning the actual data.", - default=False, + delete = BooleanVar( + description="Actually delete the objects, not just their link to the design delpoyment.", + default=True, ) class Meta: # pylint: disable=too-few-public-methods @@ -31,27 +31,25 @@ class Meta: # pylint: disable=too-few-public-methods def run(self, data, commit): """Execute Decommissioning job.""" deployments = data["deployments"] - only_traceability = data["only_traceability"] + delete = data["delete"] self.log_info( message=f"Starting decommissioning of design deployments: {', '.join([instance.name for instance in deployments])}", ) for deployment in deployments: - if only_traceability: - message = "Working on resetting traceability for this Design Deployment..." + if delete: + message = "Working on deleting objects for this Design Deployment." else: - message = "Working on resetting objects for this Design Deployment..." + message = "Working on unlinking objects from this Design Deployment." self.log_info(obj=deployment, message=message) - deployment.decommission( - local_logger=get_logger(__name__, self.job_result), only_traceability=only_traceability - ) + deployment.decommission(local_logger=get_logger(__name__, self.job_result), delete=delete) - if only_traceability: - message = f"Traceability for {deployment} has been successfully removed from Nautobot." - else: + if delete: message = f"{deployment} has been successfully decommissioned from Nautobot." + else: + message = f"Objects have been successfully unlinked from {deployment}." self.log_success(message) diff --git a/nautobot_design_builder/models.py b/nautobot_design_builder/models.py index 53dc5de..293088e 100644 --- a/nautobot_design_builder/models.py +++ b/nautobot_design_builder/models.py @@ -229,7 +229,7 @@ def __str__(self): """Stringify instance.""" return f"{self.design.name} - {self.name}" - def decommission(self, *object_ids, local_logger=logger, only_traceability=False): + def decommission(self, *object_ids, local_logger=logger, delete=True): """Decommission a design instance. This will reverse the change records for the design instance and @@ -241,10 +241,10 @@ def decommission(self, *object_ids, local_logger=logger, only_traceability=False # Iterate the change sets in reverse order (most recent first) and # revert each change set. for change_set in self.change_sets.filter(active=True).order_by("-last_updated"): - if only_traceability: - change_set.deactivate() - else: + if delete: change_set.revert(*object_ids, local_logger=local_logger) + else: + change_set.deactivate() if not object_ids: content_type = ContentType.objects.get_for_model(Deployment) @@ -373,58 +373,50 @@ def log(self, model_instance, import_mode: bool): entry.changes.update(model_instance.metadata.changes) entry.save() except ChangeRecord.DoesNotExist: - # Default full_control for created objects - full_control = model_instance.metadata.created - - # This boolean signals the intention to claim existing data because - # the action is "create_or_update" and is running in import_mode - # It assumes that we will "try" to own all the objects, if are not owned - intention_to_full_control_by_importing = ( - model_instance.metadata.action in ["create_or_update", "create"] and import_mode - ) - intention_to_import = ( - model_instance.metadata.action in ["create_or_update", "create", "update"] and import_mode - ) + entry_parameters = { + "_design_object_type": content_type, + "_design_object_id": instance.id, + "changes": model_instance.metadata.changes, + "full_control": model_instance.metadata.created, + "index": self._next_index(), + } + # Deferred import as otherwise Nautobot doesn't start + from .design import ModelMetadata # pylint: disable=import-outside-toplevel,cyclic-import + + # Path when not importing, either because it's not enabled or the action is not supported for importing. + if not import_mode or model_instance.metadata.action not in ModelMetadata.IMPORTABLE_ACTION_CHOICES: + entry = self.records.create(**entry_parameters) + return entry + + # When we have intention to claim ownership (i.e. the action is CREATE_OR_UPDATE) we first try to obtain + # `full_control` over the object, thus pretending that we have created it. + # If the object is already owned with full_control by another Design Deployment, + # we acknowledge it and set `full_control` to `False`. + # TODO: Shouldn't this change record object also need to be active? + change_records_for_instance = ChangeRecord.objects.filter_by_design_object_id(_design_object_id=instance.id) + if model_instance.metadata.action == ModelMetadata.CREATE_OR_UPDATE: + entry_parameters["full_control"] = not change_records_for_instance.filter(full_control=True).exists() + + # When we don't want to assume full control, make sure we don't try to own any of the query filter values. + # We do this by removing any query filter values from the `changes` dictionary, which is the structure that + # defines which attributes are owned by the deployment. + if not entry_parameters["full_control"]: + for attribute in model_instance.metadata.query_filter_values: + entry_parameters["changes"].pop(attribute, None) + + # Check if any owned attributes exist that conflict with the changes for this instance. + # We do this by iterating over all change records that exist for this instance, ... + for record in change_records_for_instance: + # ...iterating over all attributes in those instances changes... + for attribute in record.changes: + # ...and, finally, by raising an error if any of those overlap with those attributes that we are + # trying to own by importing the object. + if attribute in entry_parameters["changes"]: + raise ValueError( # pylint: disable=raise-missing-from + f"The {attribute} attribute for {instance} is already owned by Design Deployment {record.change_set.deployment}" + ) - # When we have intention to claim ownership, the first try is to get a full_control - # of the object, in fact, assume that we would have created it. - # If the object was already owned with full_control by another Design Deployment, - # we acknowledge it and set it to full_control=False, if not, True. - - change_record = ChangeRecord.objects.filter_by_design_object_id( - _design_object_id=instance.id, full_control=True - ).first() - if intention_to_full_control_by_importing and change_record: - if model_instance.metadata.action == "create": - raise ValueError( # pylint: disable=raise-missing-from - f"The design requires importing {instance} but is already owned by Design Deployment {change_record.change_set.deployment}" - ) - full_control = False - elif intention_to_full_control_by_importing: - full_control = True - - # Independently of having full_control or not, we check that all the attributes - # we claim as ours are not tracked by another design - if intention_to_import: - if not full_control: - for attribute in model_instance.metadata.query_filter_values: - if attribute in model_instance.metadata.changes: - del model_instance.metadata.changes[attribute] - - for record in ChangeRecord.objects.filter_by_design_object_id(_design_object_id=instance.id): - for attribute in record.changes: - if attribute in model_instance.metadata.changes: - raise ValueError( # pylint: disable=raise-missing-from - f"The {attribute} attribute for {instance} is already owned by Design Deployment {record.change_set.deployment}" - ) - - entry = self.records.create( - _design_object_type=content_type, - _design_object_id=instance.id, - changes=model_instance.metadata.changes, - full_control=full_control, - index=self._next_index(), - ) + entry = self.records.create(**entry_parameters) return entry def revert(self, *object_ids, local_logger: logging.Logger = logger): diff --git a/nautobot_design_builder/tests/designs/templates/simple_design_4.yaml.j2 b/nautobot_design_builder/tests/designs/templates/simple_design_4.yaml.j2 index 6a648e7..eb7725d 100644 --- a/nautobot_design_builder/tests/designs/templates/simple_design_4.yaml.j2 +++ b/nautobot_design_builder/tests/designs/templates/simple_design_4.yaml.j2 @@ -1,3 +1,19 @@ --- manufacturers: - "name": "Test Manufacturer" + - "!create_or_update:name": "Test Manufacturer" + "description": "Test description" +device_roles: + - "!create_or_update:name": "Switch" + "description": "Test description" +device_types: + - "!create_or_update:model": "Test Device Type" + "manufacturer__name": "Test Manufacturer" +sites: + - "!create_or_update:name": "Test Site" + "status__name": "Active" +devices: + - "!create_or_update:name": "Test Device" + "device_role__name": "Switch" + "device_type__model": "Test Device Type" + "site__name": "Test Site" + "status__name": "Active" diff --git a/nautobot_design_builder/tests/designs/test_designs.py b/nautobot_design_builder/tests/designs/test_designs.py index 489a528..099a604 100644 --- a/nautobot_design_builder/tests/designs/test_designs.py +++ b/nautobot_design_builder/tests/designs/test_designs.py @@ -112,11 +112,11 @@ class Meta: # pylint: disable=too-few-public-methods design_mode = DesignModeChoices.DEPLOYMENT -class SimpleDesignDeploymentModeCreate(DesignJob): - """Simple design job in deployment mode for 'create'.""" +class SimpleDesignDeploymentModeMultipleObjects(DesignJob): + """Simple design job in deployment mode with multiple objects.""" class Meta: # pylint: disable=too-few-public-methods - name = "Simple Design in deployment mode with create" + name = "Simple Design in deployment mode with multiple objects." "" design_file = "templates/simple_design_4.yaml.j2" design_mode = DesignModeChoices.DEPLOYMENT diff --git a/nautobot_design_builder/tests/test_decommissioning_job.py b/nautobot_design_builder/tests/test_decommissioning_job.py index 39d9426..be10558 100644 --- a/nautobot_design_builder/tests/test_decommissioning_job.py +++ b/nautobot_design_builder/tests/test_decommissioning_job.py @@ -131,7 +131,7 @@ def test_basic_decommission_run_with_full_control(self): ) record.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(0, Secret.objects.count()) @@ -159,7 +159,7 @@ def test_decommission_run_with_dependencies(self): self.assertRaises( ValueError, self.job.run, - {"deployments": [self.deployment], "only_traceability": False}, + {"deployments": [self.deployment], "delete": True}, True, ) @@ -188,7 +188,7 @@ def test_decommission_run_with_dependencies_but_decommissioned(self): self.deployment_2.decommission() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(0, Secret.objects.count()) @@ -204,7 +204,7 @@ def test_basic_decommission_run_without_full_control(self): ) record_1.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(1, Secret.objects.count()) @@ -223,7 +223,7 @@ def test_decommission_run_without_full_control_string_value(self): ) record.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(1, Secret.objects.count()) self.assertEqual("previous description", Secret.objects.first().description) @@ -240,7 +240,7 @@ def test_decommission_run_without_full_control_dict_value_with_overlap(self): ) record.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(self.initial_params, Secret.objects.first().parameters) @@ -259,7 +259,7 @@ def test_decommission_run_without_full_control_dict_value_without_overlap(self): ) record.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(self.initial_params, Secret.objects.first().parameters) @@ -283,7 +283,7 @@ def test_decommission_run_without_full_control_dict_value_with_new_values_and_ol self.secret.parameters = {**self.changed_params, **new_params} self.secret.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual({**self.initial_params, **new_params}, Secret.objects.first().parameters) @@ -299,7 +299,7 @@ def test_decommission_run_with_pre_hook_pass(self): ) record_1.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": False}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": True}, commit=True) self.assertEqual(0, Secret.objects.count()) models.Deployment.pre_decommission.disconnect(fake_ok) @@ -318,7 +318,7 @@ def test_decommission_run_with_pre_hook_fail(self): self.assertRaises( DesignValidationError, self.job.run, - {"deployments": [self.deployment], "only_traceability": False}, + {"deployments": [self.deployment], "delete": True}, True, ) @@ -351,13 +351,11 @@ def test_decommission_run_multiple_deployment(self): self.assertEqual(2, Secret.objects.count()) - self.job.run( - data={"deployments": [self.deployment, self.deployment_2], "only_traceability": False}, commit=True - ) + self.job.run(data={"deployments": [self.deployment, self.deployment_2], "delete": True}, commit=True) self.assertEqual(0, Secret.objects.count()) - def test_decommission_run_with_only_traceability(self): + def test_decommission_run_without_delete(self): self.assertEqual(1, Secret.objects.count()) record = models.ChangeRecord.objects.create( @@ -368,7 +366,7 @@ def test_decommission_run_with_only_traceability(self): ) record.validated_save() - self.job.run(data={"deployments": [self.deployment], "only_traceability": True}, commit=True) + self.job.run(data={"deployments": [self.deployment], "delete": False}, commit=True) self.assertEqual(1, Secret.objects.count()) record.refresh_from_db() diff --git a/nautobot_design_builder/tests/test_design_job.py b/nautobot_design_builder/tests/test_design_job.py index ca28c05..942fac9 100644 --- a/nautobot_design_builder/tests/test_design_job.py +++ b/nautobot_design_builder/tests/test_design_job.py @@ -1,7 +1,7 @@ """Test running design jobs.""" import copy -from unittest.mock import patch, Mock, ANY +from unittest.mock import patch, Mock, ANY, MagicMock from django.core.exceptions import ValidationError @@ -121,28 +121,27 @@ def test_import_design_update(self): "The description attribute for Test Manufacturer is already owned by Design Deployment Simple Design in deployment mode with update - deployment name example", ) - def test_import_design_create(self): - """Confirm that existing data can be imported with 'create'.""" - job = self.get_mocked_job(test_designs.SimpleDesignDeploymentModeCreate) + def test_import_design_multiple_objects(self): + """Confirming that multiple, interrelated objects can be imported.""" + job = self.get_mocked_job(test_designs.SimpleDesignDeploymentModeMultipleObjects) - # The object to be imported by the design deployment already exists - manufacturer = Manufacturer.objects.create(name="Test Manufacturer") - self.data["import_mode"] = True - self.data["deployment_name"] = "deployment name example" + # Create data initially + self.data["deployment_name"] = "I will be deleted" job.run(data=self.data, commit=True) self.assertJobSuccess(job) - self.assertEqual(Deployment.objects.first().name, "deployment name example") - self.assertEqual(ChangeRecord.objects.first().design_object, manufacturer) - # Running the import twice for a 'create' operation should raise an exception - job = self.get_mocked_job(test_designs.SimpleDesignDeploymentModeCreate) - self.data["deployment_name"] = "another deployment name example" - with self.assertRaises(ValueError) as error: - job.run(data=self.data, commit=True) - self.assertEqual( - str(error.exception), - "The design requires importing Test Manufacturer but is already owned by Design Deployment Simple Design in deployment mode with create - deployment name example", - ) + # Unlink the objects from the deployment so that they can be re-imported + deployment = Deployment.objects.get(name=self.data["deployment_name"]) + deployment.decommission(local_logger=MagicMock(), delete=False) + deployment.delete() + + self.data["import_mode"] = True + self.data["deployment_name"] = "I will persist" + job.run(data=self.data, commit=True) + + self.assertJobSuccess(job) + self.assertEqual(ChangeRecord.objects.count(), 6) + self.assertTrue(ChangeRecord.objects.filter_by_design_object_id(Device.objects.first().pk).exists()) class TestDesignJobLogging(DesignTestCase):