Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replication: update LastSyncTime if its not nill #260

Merged
merged 3 commits into from
Nov 2, 2022

Conversation

Madhu-1
Copy link
Member

@Madhu-1 Madhu-1 commented Oct 28, 2022

LastSyncTime can be optional and nil also, there is no strict check for it, and if we dont have this check the default UNIX time will get added to the CR, which doesn't make sense. If the time is not present keep the last known LastSyncTime itself.

Depends-On: csi-addons/spec#47

Copy link

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works to not update last known lastSyncTime in cases where the call does not return a value, or hold it at epoch till it does.

NOTE: As I have not cross-checked other areas of code not approving as such.

yati1998
yati1998 previously approved these changes Oct 31, 2022
@Madhu-1
Copy link
Member Author

Madhu-1 commented Oct 31, 2022

@nixpanic PTAL

Copy link
Collaborator

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this -1 until the discussion in csi-addons/spec#47 is resolved.

@mergify mergify bot dismissed yati1998’s stale review November 1, 2022 14:03

Pull request has been modified.

@mergify mergify bot added the vendor Pull requests that update vendored dependencies label Nov 1, 2022
Copy link

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Madhu-1
Copy link
Member Author

Madhu-1 commented Nov 2, 2022

@Mergifyio rebase

LastSyncTime can be optional and nil also, there
is no strict check for it and if we dont have this
check the default UNIX time will get added to the
CR which doesnt make sense. If the time is not
present keeping the last known LastSyncTime itself.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
csi-addons/spec#47
has the defined errors for the
GetVolumeReplicationInfo RPC call.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Added known error for GetVolumeReplicationInfo
RPC call as per the predefined error messages
in the csiaddons spec.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@mergify
Copy link

mergify bot commented Nov 2, 2022

rebase

✅ Branch has been successfully rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vendor Pull requests that update vendored dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants