[ltm-1.6] Import mode for existing data #178

chadell · 2024-07-05T09:28:11Z

This PR adds a new feature to import existing data into a new Design Deployment

TODOs:

Check why identifiers are not included in the imported change record
Include more actions other than "create_and_update", cover the "create" and "update" (for attributes only, not full control)
Create a queryset used many times
Add Docs for the new functionality
Add a functionality to remove traceability when decommissioning a design deployment

data

abates · 2024-07-24T11:24:29Z

nautobot_design_builder/design.py

+                if self.environment.import_mode and self.metadata.action != ModelMetadata.UPDATE:
+                    self.metadata.attributes.update(field_values)
+


What are the circumstances that would cause this block of code to be executed? If the builder is not running in import mode, then this block is reached from a create_or_update when the requested object already exists and will now be updated. With this change I think we essentially treat all create actions as create_or_update when in import mode, but I doubt that there is even a query filter for most cases of create. If a design is intended to be able to be run in import mode then there shouldn't be any actions that are create only, they should be create_or_update in order to identify the query fields.

abates · 2024-07-24T11:26:45Z

nautobot_design_builder/design.py

@@ -842,7 +852,7 @@ def resolve_values(self, value: Union[list, dict, str]) -> Any:
                value[k] = self.resolve_value(item)
        return value

-    def _create_objects(self, model_class: Type[ModelInstance], objects: Union[List[Any], Dict[str, Any]]):
+    def _create_or_import_objects(self, model_class: Type[ModelInstance], objects: Union[List[Any], Dict[str, Any]]):


while this method is probably poorly named, I'm not sure that this PR should be where we rename it. Every change we have in the ltm branch is something we'll have to figure out how to pull over to the develop branch (which still doesn't have the lifecycle changes in it). I'd prefer to keep changes to absolute minimums in order to make the updates to develop easier down the road.

That's a valid point. I will exchange it by a comment about it

abates · 2024-07-24T11:37:29Z

nautobot_design_builder/design_job.py

@@ -282,6 +294,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)


Suggested change

data["import_mode"] = self.determine_import_mode(data)

data["import_mode"] = self.is_deployment_job() and data.get("import_mode", False)

abates · 2024-07-24T11:37:46Z

nautobot_design_builder/design_job.py

+    @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"]
+


Suggested change

@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"]

I thought it would be clearer to warp in this function similar to what we had for deployment name...

abates · 2024-07-24T11:39:00Z

nautobot_design_builder/design_job.py

+        if data["import_mode"] and data["deployment_name"]:
+            self.log_info(message=f'Running in import mode for {data["deployment_name"]}.')
+


Suggested change

if data["import_mode"] and data["deployment_name"]:

self.log_info(message=f'Running in import mode for {data["deployment_name"]}.')

if data["import_mode"]:

self.log_info(message=f'Running in import mode for {data["deployment_name"]}.')

The second expression in the conditional is unneeded because import mode cannot be True if the design is not a deployment job.

abates · 2024-07-24T11:48:37Z

Before I can approve this I'd like to see a test (or confirm that there is a test) that does the following:

Creates a bunch of objects outside of a design
Runs a design in import mode that will create some objects that were created already in step 1. The portions of the design that correspond to step 1 should have no action tags (default create).
Confirms that there are no errors and that the objects that should have been imported now show as "owned" by the design.

abates · 2024-07-24T11:50:19Z

nautobot_design_builder/jobs.py

@@ -27,14 +31,29 @@ class Meta:  # pylint: disable=too-few-public-methods
    def run(self, data, commit):
        """Execute Decommissioning job."""
        deployments = data["deployments"]
+        only_traceability = data["only_traceability"]


We need a different name than only_traceability. It is not clear what the meaning of that variable is without really digging into the code. I'll give this some thought.

abates · 2024-07-24T11:51:07Z

nautobot_design_builder/models.py

@@ -368,11 +373,56 @@ def log(self, model_instance):
            entry.changes.update(model_instance.metadata.changes)
            entry.save()
        except ChangeRecord.DoesNotExist:
+            # Default full_control for created objects


Please add a detailed code comment here explaining this change. It seems complex and I'm not sure what the intent is.

chadell · 2024-07-25T07:55:06Z

Before I can approve this I'd like to see a test (or confirm that there is a test) that does the following:

Creates a bunch of objects outside of a design

Runs a design in import mode that will create some objects that were created already in step 1. The portions of the design that correspond to step 1 should have no action tags (default create).

Confirms that there are no errors and that the objects that should have been imported now show as "owned" by the design.

This test https://github.com/nautobot/nautobot-app-design-builder/pull/178/files#diff-a45426cea1233c01ee8f3f6db38123c56701edf1f027f129206094df76415bffR124 does it but it's not "a bunch" but just one object. Do you think is relevant to have more than one?

Kircheneer · 2024-07-29T08:31:07Z

Further open points

Improve error handling for already claimed items on import mode (no exception directly bubbled up to job context)

Kircheneer · 2024-07-31T12:49:58Z

Closing in favor of #181

feat: Import functionality to boostrap Design Deployments from existing

1abd402

data

chadell requested review from abates and mzbroch as code owners July 5, 2024 09:28

chadell added 4 commits July 5, 2024 12:07

docs: document feature

679d371

chore: simplify queryset

62f2dcd

feat: refine all the import methods

7ec75fb

feat: allow the only_traceability mode for decommissioning

800b691

chadell force-pushed the 1.6_import_state branch from ce4f519 to 800b691 Compare July 9, 2024 09:34

abates reviewed Jul 24, 2024

View reviewed changes

Kircheneer mentioned this pull request Jul 29, 2024

[ltm-1.6] Import mode for existing data (Leo) #181

Merged

Kircheneer closed this Jul 31, 2024

abates deleted the 1.6_import_state branch October 4, 2024 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ltm-1.6] Import mode for existing data #178

[ltm-1.6] Import mode for existing data #178

chadell commented Jul 5, 2024 •

edited

Loading

abates Jul 24, 2024

abates Jul 24, 2024

chadell Jul 25, 2024

abates Jul 24, 2024

abates Jul 24, 2024

chadell Jul 25, 2024

abates Jul 24, 2024

abates commented Jul 24, 2024 •

edited

Loading

abates Jul 24, 2024

abates Jul 24, 2024

chadell commented Jul 25, 2024

Kircheneer commented Jul 29, 2024

Kircheneer commented Jul 31, 2024

		if self.environment.import_mode and self.metadata.action != ModelMetadata.UPDATE:
		self.metadata.attributes.update(field_values)

		@@ -282,6 +294,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.determine_import_mode(data)
	data["import_mode"] = self.is_deployment_job() and data.get("import_mode", False)

		if data["import_mode"] and data["deployment_name"]:
		self.log_info(message=f'Running in import mode for {data["deployment_name"]}.')

[ltm-1.6] Import mode for existing data #178

[ltm-1.6] Import mode for existing data #178

Conversation

chadell commented Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abates commented Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadell commented Jul 25, 2024

Kircheneer commented Jul 29, 2024

Further open points

Kircheneer commented Jul 31, 2024

chadell commented Jul 5, 2024 •

edited

Loading

abates commented Jul 24, 2024 •

edited

Loading