Ssot snow delete function #263
Conversation
collin-wicker
requested review from
glennmatthews,
qduk and
a team
as code owners
jdrew82
added
type: minor feature
integration: servicenow
There was a problem hiding this comment.
Thanks for the PR! It would probably be good if @glennmatthews could take a look as well
| # description="Delete records from ServiceNow if not present in Nautobot", | ||
| # default=False, | ||
| # ) | ||
| delete_records = BooleanVar( |
There was a problem hiding this comment.
There are several areas in the models.py file that use diffsync.job.logger.debug but I believe I defaulted the debug value to True in the jobs.py file when I was testing and must have forgotten to set it back to False.
There was a problem hiding this comment.
I have removed the "default=True" for the delete checkbox in the latest commits.
| self.diffsync.objects_to_delete[self._modelname].append(_object) | ||
| print(self.diffsync.objects_to_delete[self._modelname]) | ||
| super().delete() | ||
| except Exception as exception: |
There was a problem hiding this comment.
Can you narrow this exception down a little so its not a blanket except Exception?
There was a problem hiding this comment.
After updating the code in the latest commit I don't believe a try/except clause is necessary here as I'm not having any issues with errors in my tests. I have eliminated this try/except clause in the latest commits.
| if value is not None: | ||
| # Cacheing disabled as it was introducing instability and unpredictable results during delete method testing |
There was a problem hiding this comment.
Did you figure out why this was the case? Can we possibly fix the cache mechanism instead? I reckon its existence is probably necessary for some bigger deployments. Also if we do decide to omit caching, can you delete the commented out code?
There was a problem hiding this comment.
I figured out what the issue with the caching functionality was and handled it in the new commits. Please review the newest commits for the changes.
Fixed caching mechanism, removed try/except block from delete as it w…
Merging changes made for linting tests
| if value is not None: | ||
| # Cacheing disabled as it was introducing instability and unpredictable results during delete method testing |
There was a problem hiding this comment.
I figured out what the issue with the caching functionality was and handled it in the new commits. Please review the newest commits for the changes.
| self.diffsync.objects_to_delete[self._modelname].append(_object) | ||
| print(self.diffsync.objects_to_delete[self._modelname]) | ||
| super().delete() | ||
| except Exception as exception: |
There was a problem hiding this comment.
After updating the code in the latest commit I don't believe a try/except clause is necessary here as I'm not having any issues with errors in my tests. I have eliminated this try/except clause in the latest commits.
| # description="Delete records from ServiceNow if not present in Nautobot", | ||
| # default=False, | ||
| # ) | ||
| delete_records = BooleanVar( |
There was a problem hiding this comment.
I have removed the "default=True" for the delete checkbox in the latest commits.
Added delete functionality to ServiceNowCRUDMixin in models.py. Disabled cacheing functionality in models.py as it was causing unpredictable results when testing deleting records from ServiceNow. Tested this code with a ServiceNow personal development environment and everything worked as intended in the current format.
I was unsure as to whether the delete functionality should be available by default in the ServiceNow job but it is currently available by default. Please contact me with any questions/concerns.