-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add snapshot time #10854
Add snapshot time #10854
Conversation
✔️ Deploy Preview for infallible-bardeen-164bc9 ready! 🔨 Explore the source changes: 58cafd4 🔍 Inspect the deploy log: https://app.netlify.com/sites/infallible-bardeen-164bc9/deploys/61d0c585cf4f1200079b7063 😎 Browse the preview: https://deploy-preview-10854--infallible-bardeen-164bc9.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Viktor, Great work on these changes. They look good. Left one minor comment. Also, it'd be great if you can attach a screenshot of how the list_snapshots
command looks like both without and with your change since this is user-facing. Also, if this is the first time you are contributing then it will be great to add your name to contributors file
Thank you!
Hi @sanketkedia and @OlegLoginov I have fixes the suggested errors and have made a screenshot with the 2 outputs of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @viko600, Some comments on code style, and then we are good to go.
I want to take another issue after this one is resolved. Should I only look for issues with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@viko600 Backup tests are failing due to this change because they expect a strict format. I'll commit the change to fix them, run tests and then we'll probably be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viko600 Tests look good, so accepting this PR. Also, cases like the behavior when the snapshot is in "CREATING" or "DELETING" state also work. I will merge it.
@viko600 We don't necessarily restrict from our end but the |
Added snapshot creation time when the yb-admin list snapshot command is issuedalaigned