Fix: Add proper onedrive sync status to api #90
Conversation
Signed-off-by: Daishan Peng <[email protected]>
| log.Errorf("failed to open metadata file: %v", err) | ||
| continue | ||
| } | ||
| defer file.Close() |
There was a problem hiding this comment.
Using defer inside of a for-loop will leak resources until the for-loop is completely done.
| log.Errorf("failed to decode metadata file: %v", err) | ||
| continue | ||
| } | ||
| if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { |
There was a problem hiding this comment.
If this runs before the controllers finish, then this will mess things up. Specifically, we MUST make sure that oneDriveLinks.Status.RunName is set before trying to update the status here. Also, if oneDriveLinks.Status.RunName goes blank, then that means the run is done and we don't need to overwrite the status again. After you get the object, can you check this field and return nil if the RuName isn't set?
Lastly, we don't want to update the status of the OneDriveLinks object unnecessarily because that will cause the controllers to run again. Can you check that the Status and Error fields are set before overwriting them?
| log.Errorf("failed to open metadata file: %v", err) | ||
| continue | ||
| } | ||
| func() { |
There was a problem hiding this comment.
I wasn't expecting this approach, but I like it.
There was a problem hiding this comment.
chatgpt all the way!
| }() | ||
|
|
||
| go func(ctx context.Context) { |
There was a problem hiding this comment.
I realized something this morning (not sure why it popped in my head). The controller below essentially does the same thing except that it waits for the run status to be in a terminal state before it does it.
Instead of having this go-routine, you could incorporate this logic into the next handler. Read the metadata file and put the information on the status of the oneDriveLinks object, but stop there unless the run is in a terminal state. This would get rid of the weirdness of the conflicts, retries, and ensuring that the RunName is set.
Feel free to merge as-is, since I already approved. I can incorporate it later.
There was a problem hiding this comment.
Yeah, this is a better way to handle that. Moved 👍
StrongMonkey
force-pushed
the
add-onedrive-status
branch
2 times, most recently
from
327cb33 to
b446cca
Compare
Signed-off-by: Daishan Peng <[email protected]>
StrongMonkey
force-pushed
the
add-onedrive-status
branch
from
b446cca to
3b78453
Compare
| var thread v1.Thread | ||
| if err := req.Get(&thread, oneDriveLinks.Namespace, oneDriveLinks.Status.ThreadName); apierrors.IsNotFound(err) { | ||
| // Might not be in the cache yet. | ||
| return nil | ||
| } else if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This is all duplicated below. At the very least, can we pull the getting of the thread out of this if-statement and remove the place below where we get the thread.
Signed-off-by: Daishan Peng <[email protected]>
This PR adds proper syncing status from onedrive tool to api so that UI can display the status when it is syncing files.