You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In 1.9.0, the results of a sync are stored as a field in DiffSyncModel, and for most users this status is automatically set when calling the super() of the create()/update()/delete() methods.
There are a few things that are awkward about this API:
Providing a custom success message requires setting the status twice, once by using super().create/update/delete, and then again by using self.set_status() to override what was put there by default.
create/update/delete are required to return self for success or None for an error (and to skip children).
In the success case, it's unclear as to why self needs to be returned. IE: why is this just not a bool?
In the failure case, it's unclear how to provide a failure message.
At a conceptual level, it's also a bit weird to have the status stored on an object. In particular, how do you report the result of a delete()? If that object is no longer in the destination backend, then how can I read the result of that after a sync process? I think this is ultimately because the result is not a property of an object, it's the result of a process.
This Feature Request proposes:
That the sync status is stored in the Diff (or part of the diff tree) instead of DiffSyncModel
That the status be an object instead of two distinct values (status enum and message)
That the status can be sub-classed by diffsync users to add additional context
That the create/update/delete signature in DiffSyncModel be altered to return the status instead of the object
That the Diff tree provides a public API to read the status & message
Misc notes
It's likely this will be an appropriate issue for (2.0) 2.0 Tracking #232
The only storage engine I use is the Local (in-memory), it's unclear to me if this has an impact on the Redis storage engine.
Use Case
# DiffSync codeclassSyncResult(pydantic.BaseModel):
status: DiffSyncStatusmessage: strskip_children: bool=False# Set to True to skip updating children# User code example 1 - providing custom valuesclassMyModelA(DiffSyncModel):
@classmethoddefcreate(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) ->Tuple[Self, SyncResult]:
returncls(**ids, **atts), SyncResult(DiffSyncStatus.SUCCESS, "Sync successful")
defupdate(self, attrs:Dict[str,any]) ->SyncResult:
... # perform updatesuper().update(attrs) # Returns a successful SyncResult, but we can craft our ownreturnSyncResult(DiffSyncStatus.SUCCESS, "with immense difficulty")
defdelete(self) ->SyncResult:
returnSyncResult(DiffSyncStatus.ERROR, "Delete is not implemented")
# User code example 2 - simple user experienceclassMyModelB(DiffSyncModel):
@classmethoddefcreate(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) ->Tuple[Self, SyncResult]:
returncls(**ids, **atts)
# I'm struggling to figure out an elegant way to provide the user experience that exists today,# which is to make it easy to report success. This isn't ideal, but one thought is:# - If a single-value is returned, the returned value is the object created and success is assumed# - If a 2-tuple is returned: interpret the first value as the object created, and the second value is the SyncResultdefupdate(self, attrs:Dict[str,any]) ->SyncResult:
...
returnsuper().update(**attrs) # DiffSyncModel.update() returns a success object by defaultdefdelete(self) ->SyncResult:
returnsuper().delete() # DiffSyncModel.delete() returns a success object by default.# User code example 3 - augmented result to add additional fieldsclassCustomSyncResult(diffsync.SyncResult):
nso_dry_run_result: strclassMyModelC(DiffSyncModel):
@classmethoddefcreate(cls, diffsync:DiffSync, ids:Dict[str, any], attrs:Dict[str, any]) ->Tuple[Self, SyncResult]:
dry_run_text=nso.dry_run(...)
status=CustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
obj=cls(**ids, **atts)
returnobj, statusdefupdate(self, attrs:Dict[str,any]) ->SyncResult:
dry_run_text=nso.dry_run(...)
super().update(attrs)
returnCustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
defdelete(self) ->SyncResult:
returnCustomSyncResult(DiffSyncStatus.SUCCESS, "Sync successful", nso_dry_run_result=dry_run_text)
The most straight forward way for a user to read the result of a status is to embed that status object into the DiffElement.
After a typical sync_to/sync_from, this should be an actual SyncStatus.
Point of discussion: After a diff_to/diff_from, I'm not sure if this field should be left as None or if the diff should just report success. I lean slightly towards reporting a SUCCESS/UNKNOWN value here, since that would produce a consistent API
Point of discussion: If children is skipped because the parent took care of deleting the child objects, what should this report? The Parent's status? No status? Unknown status? A special other status?
The text was updated successfully, but these errors were encountered:
I support the idea of returning a sync result rather then the object itself, I think this would make error handling more straight-forward. @glennmatthews/@Dav-C - any opinion here?
Is anything lost by not returning the object? I think the current implementation loosely follows the REST paradigm where the object is returned for immediate use and the lack of a returned object is an error that should be handled. In any case, I think the return from all the methods should be consistent to avoid confusion.
When calling update or delete, you are calling on the class instance itself, so you have the instance available in 100% of cases. The proposal for create returns a tuple with the class and the SyncResult, so that's covered as well
Environment
Proposed Functionality
In 1.9.0, the results of a sync are stored as a field in DiffSyncModel, and for most users this status is automatically set when calling the super() of the create()/update()/delete() methods.
There are a few things that are awkward about this API:
self
for success orNone
for an error (and to skip children).self
needs to be returned. IE: why is this just not a bool?At a conceptual level, it's also a bit weird to have the status stored on an object. In particular, how do you report the result of a delete()? If that object is no longer in the destination backend, then how can I read the result of that after a sync process? I think this is ultimately because the result is not a property of an object, it's the result of a process.
This Feature Request proposes:
Misc notes
Use Case
The most straight forward way for a user to read the result of a status is to embed that status object into the DiffElement.
The text was updated successfully, but these errors were encountered: