-
Notifications
You must be signed in to change notification settings - Fork 93
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
feat(backups): change list instance backups response payload #5061
Conversation
this.setState({ | ||
snapshots: snapshots?.sort((a, b) => | ||
snapshots: response.backups?.sort((a, b) => |
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.
This just reverts what we've done in - https://github.com/replicatedhq/kots/pull/5033/files#diff-84ad0b83d6595420183ed4f5d5f3b47be290c7a5a014f1cc4729329c2ac3dc2a
The main difference we introduced has been 2 extra fields (the backup count and expected) and a reduction of potential status phases:
kots/pkg/kotsadmsnapshot/types/types.go
Lines 66 to 75 in 05573a5
const ( // BackupStatusInProgress indicates that the backup is currently in progress BackupStatusInProgress BackupStatus = "InProgress" // BackupStatusCompleted indicates that the backup has been completed successfully BackupStatusCompleted BackupStatus = "Completed" // BackupStatusFailed indicates that the backup has failed BackupStatusFailed BackupStatus = "Failed" // BackupStatusDeleting indicates that the backup is being deleted BackupStatusDeleting BackupStatus = "Deleting" )
I've checked in:
kots/web/src/components/snapshots/SnapshotRow.jsx
Lines 36 to 142 in 5fbcb34
render() { const { snapshot, app, hideRestore } = this.props; const isExpired = dayjs(new Date()).isSameOrAfter(snapshot?.expiresAt); return ( <div className={`flex flex-auto SnapshotRow--wrapper card-item alignItems--center u-padding--15 u-marginTop--10 clickable ${ snapshot?.status === "Deleting" && "is-deleting" } ${snapshot?.status === "InProgress" && "in-progress"} ${ isExpired && "is-expired" }`} onClick={() => this.handleSnapshotClick()} > <div className="flex-column flex1" style={{ maxWidth: "700px" }}> <p className={`u-fontSize--largest ${ isExpired || snapshot?.status === "Deleting" ? "u-textColor--bodyCopy" : "card-item-title" } u-lineHeight--normal u-fontWeight--bold u-marginRight--10`} > {snapshot?.name} </p> <div className="flex flex1 alignItems--center u-marginTop--10"> <p className="u-fontSize--small u-textColor--bodyCopy u-fontWeight--medium u-lineHeight--normal u-marginRight--20"> {snapshot?.startedAt ? Utilities.dateFormat( snapshot?.startedAt, "MMM D YYYY @ hh:mm a z" ) : "n/a"} </p> </div> </div> <div className="flex flex1"> <div className="flex flex-auto alignItems--center u-marginTop--5"> <div className="flex flex1 flex-column"> <div className="flex justifyContent--flexStart" style={{ gap: "60px" }} > {snapshot?.volumeSizeHuman && ( <p className="u-fontSize--normal u-textColor--accent u-fontWeight--bold u-lineHeight--normal justifyContent--center flex alignItems--center"> <span className="icon snapshot-volume-size-icon" />{" "} {snapshot?.volumeSizeHuman}{" "} </p> )} <p className="u-fontSize--normal u-textColor--accent u-fontWeight--bold u-lineHeight--normal justifyContent--center flex alignItems--center"> <span className="icon snapshot-volume-icon" />{" "} {snapshot?.volumeSuccessCount}/{snapshot?.volumeCount} </p> </div> {snapshot?.status === "Completed" ? ( <p className="u-fontSize--small u-textColor--bodyCopy u-fontWeight--medium u-lineHeight--normal u-marginTop--10 u-marginRight--20"> <span className={`status-indicator u-marginRight--5 ${snapshot?.status.toLowerCase()}`} > {Utilities.snapshotStatusToDisplayName(snapshot?.status)} </span> on{" "} {snapshot?.finishedAt ? snapshot?.finishedAt ? Utilities.dateFormat( snapshot?.finishedAt, "MMM D YYYY @ hh:mm a z" ) : "TBD" : "n/a"} </p> ) : ( <span className={`status-indicator u-marginTop--10 u-marginRight--5 ${snapshot?.status.toLowerCase()}`} > {Utilities.snapshotStatusToDisplayName(snapshot?.status)} </span> )} </div> </div> </div> {!isExpired && snapshot?.status !== "Deleting" && ( <div className="flex flex-auto"> {snapshot?.status === "Completed" && !hideRestore && ( <div className="flex"> <Icon icon="sync" size={20} className="clickable" onClick={(e) => this.handleRestoreClick(e, snapshot)} data-tip="Restore from this backup" /> <ReactTooltip effect="solid" className="replicated-tooltip" /> </div> )} {snapshot?.status !== "InProgress" && ( <Icon icon="trash" size={20} className="clickable u-marginLeft--20 error-color" onClick={(e) => this.handleDeleteClick(e, snapshot)} /> )} </div> )} </div> ); } }
And it doesn't look like we've introduced anything different or non standard to what we're already using ☝️ CC @miaawong if you could confirm 👀
func TestRollupStatus(t *testing.T) { | ||
tests := []struct { | ||
backupStatuses []BackupStatus | ||
expected BackupStatus | ||
}{ | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusInProgress, | ||
BackupStatusInProgress, | ||
}, | ||
expected: BackupStatusInProgress, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusInProgress, | ||
BackupStatusFailed, | ||
}, | ||
expected: BackupStatusInProgress, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusInProgress, | ||
BackupStatusCompleted, | ||
}, | ||
expected: BackupStatusInProgress, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusInProgress, | ||
BackupStatusDeleting, | ||
}, | ||
expected: BackupStatusInProgress, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusDeleting, | ||
BackupStatusDeleting, | ||
}, | ||
expected: BackupStatusDeleting, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusDeleting, | ||
BackupStatusFailed, | ||
}, | ||
expected: BackupStatusDeleting, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusDeleting, | ||
BackupStatusCompleted, | ||
}, | ||
expected: BackupStatusDeleting, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusFailed, | ||
BackupStatusFailed, | ||
}, | ||
expected: BackupStatusFailed, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusFailed, | ||
BackupStatusCompleted, | ||
}, | ||
expected: BackupStatusFailed, | ||
}, | ||
{ | ||
backupStatuses: []BackupStatus{ | ||
BackupStatusCompleted, | ||
BackupStatusCompleted, | ||
}, | ||
expected: BackupStatusCompleted, | ||
}, | ||
} |
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.
This encodes the matrix states we talked about in the design doc here:
// number of velero backups expected to exist for the Backup to be considered done | ||
ExpectedBackupCount int `json:"expectedBackupCount"` | ||
// number of velero backups that actually exist | ||
BackupCount int `json:"backupCount"` |
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.
With us no longer having the ReplicatedBackup
struct I believe these 2 fields end up being useful, specifically when we convey Failed
states it's possible for the client to check if the counts match. They weren't part of the design doc proposal but I ended up adding them as I worked through the code.
What this PR does / why we need it:
As per our design doc - https://docs.google.com/document/d/1JVgVSnrPWeLhS3qm3CC_MdDGDvFpM5TSABkySbHum-w/edit?tab=t.0#heading=h.ocsy1d20qe90 - we're changing the list instance backup endpoint again to better convey the 2 backup system we've designed.
Which issue(s) this PR fixes:
https://app.shortcut.com/replicated/story/117470/change-the-listinstancebackups-endpoint-to-return-a-flat-array-structure-of-replicated-backups
Does this PR require a test?
Tests added
Also run some manual tests with an EC cluster to validate current functionality remains unchanged:
Does this PR require a release note?
Does this PR require documentation?
NONE