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

feat(backups): change list instance backups response payload #5061

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

JGAntunes
Copy link
Member

@JGAntunes JGAntunes commented Dec 18, 2024

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:

image

image

Does this PR require a release note?

NONE

Does this PR require documentation?

NONE

@JGAntunes JGAntunes added the type::feature New feature or request label Dec 18, 2024
@JGAntunes JGAntunes self-assigned this Dec 18, 2024
this.setState({
snapshots: snapshots?.sort((a, b) =>
snapshots: response.backups?.sort((a, b) =>
Copy link
Member Author

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:

  • 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:

  • 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 👀

Comment on lines +12 to +87
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,
},
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +90 to +93
// 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"`
Copy link
Member Author

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.

@JGAntunes JGAntunes requested a review from emosbaugh December 18, 2024 15:48
@JGAntunes JGAntunes marked this pull request as ready for review December 18, 2024 15:48
@JGAntunes JGAntunes merged commit 0b5c8e4 into main Dec 18, 2024
122 checks passed
@JGAntunes JGAntunes deleted the feat/list_instance_backups branch December 18, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants