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 Snapshots to VM Details #5954
Add Snapshots to VM Details #5954
Conversation
a053118
to
50beb4d
Compare
2a6bfe8
to
c87d1f2
Compare
c87d1f2
to
a13a835
Compare
|
||
const submit = async (e) => { | ||
e.preventDefault(); | ||
const newSnapshot: VMSnapshot = { |
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.
could you please create a wrapper for these?
return ( | ||
<TableRow id={name} index={index} trKey={name} style={style}> | ||
<TableData className={dimensify()}> | ||
<ValidationCell validation={validation.name}> |
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.
I suppose we probably don't need validations yet, but it probably doesn't hurt to have it setup for potential future.
name: getName(snapshot), | ||
namespace: getNamespace(snapshot), | ||
date: getCreationTimestamp(snapshot), | ||
status: getCondition(getStatusConditions(snapshot), READY)?.status, |
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.
the status is more complex.
- There are two (Ready and Progressing) conditions which should be read and final status created.
- we should also account for the Error field and potentially ReadyToUse one as well
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.
it could be worth creating an enum for the final status used by UI
const snapshotsRowData = data.map((snapshot) => ({ | ||
name: getName(snapshot), | ||
namespace: getNamespace(snapshot), | ||
date: getCreationTimestamp(snapshot), |
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.
can we call it creationTimestamp as well since date is too generic?
loaded, | ||
loadError, | ||
}) => { | ||
const snapshotsRowData = data.map((snapshot) => ({ |
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.
there should be metadata.name
and metadata.uid
field for the table to work properly
afc0e69
to
a782f3b
Compare
@suomiy fixed suggestions, added wrapper |
70564b7
to
0253a12
Compare
transforms: [sortable], | ||
}, | ||
{ | ||
title: 'Date', |
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.
convention in console is Created
</ValidationCell> | ||
</TableData> | ||
<TableData className={dimensify()}> | ||
<ValidationCell validation={validation.date}> |
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.
creationTimestamp ? maybe it would be easier to remove the validations after all
</TableData> | ||
<TableData className={dimensify()}> | ||
<ValidationCell validation={validation.status}> | ||
<Status status={snapshot?.status?.readyToUse ? 'Ready' : 'Not Ready'} /> |
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.
statuses still need fixing + good filtering
|
||
export type SnapshotSimpleDataValidation = { | ||
name?: ValidationObject; | ||
date?: ValidationObject; |
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.
+here
6e2f9a8
to
46ec70d
Compare
Added initial snapshot name - |
82dac98
to
a4d3684
Compare
ca82387
to
f69c71a
Compare
f69c71a
to
3321e2c
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: glekner, yaacov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Minimal Implementation of VM Snapshots.
There was no way to filter snapshots with the current DefaultPage because
fieldSelector
doesn't supportspec.source.name
, so I added a filter function prop.