Skip to content

replication: update LastSyncTime if its not nill#260

Merged
mergify[bot] merged 3 commits into
csi-addons:mainfrom
Madhu-1:fix-last-sync-time
Nov 2, 2022
Merged

replication: update LastSyncTime if its not nill#260
mergify[bot] merged 3 commits into
csi-addons:mainfrom
Madhu-1:fix-last-sync-time

Conversation

@Madhu-1

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

Copy link
Copy Markdown
Member

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

@ShyamsundarR ShyamsundarR left a comment

Copy link
Copy Markdown

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

Madhu-1 commented Oct 31, 2022

Copy link
Copy Markdown
Member Author

@nixpanic PTAL

@nixpanic nixpanic left a comment

Copy link
Copy Markdown
Member

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

@ShyamsundarR ShyamsundarR left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

@Madhu-1

Madhu-1 commented Nov 2, 2022

Copy link
Copy Markdown
Member Author

@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

mergify Bot commented Nov 2, 2022

Copy link
Copy Markdown

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the fix-last-sync-time branch from c31bad2 to 8df5465 Compare November 2, 2022 08:12
@mergify mergify Bot merged commit 5c0550d into csi-addons:main Nov 2, 2022
iPraveenParihar pushed a commit to iPraveenParihar/kubernetes-csi-addons that referenced this pull request Apr 15, 2025
Syncing latest changes from main for kubernetes-csi-addons
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