Allow restore data from sites in groups backup to different site #4509

meain · 2023-10-16T13:37:54Z

This adds the option restore a site to a different one from the one that it was backed up from.

Does this PR need a docs update or release note?

✅ Yes, it's included
🕐 Yes, but in a later PR
⛔ No

Type of change

Issue(s)

fixes Allow restoring to a different Group(site) that the one backed up from #4511

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

aviator-app · 2023-10-16T13:37:59Z

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was closed without merging. If you still want to merge this PR, re-open it.

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

sonarqubecloud · 2023-10-16T13:39:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sonarqubecloud · 2023-10-18T11:44:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.3% Duplication

meain · 2023-10-17T10:44:42Z

src/cli/utils/restore_config.go

+	SubServiceType path.ServiceType
+	SubService     string


Any thoughts on this pattern? Any alternate suggestions?

This seems to duplicate the selector's ownership. Moving these out of there and into the restore config opens up the potential for conflict between these props and the selector. Similarly, it might lead to building this data into the selector downstream anyway. Iirc, the selector already has the capacity to specify the secondary resource; seems like this identification could be owned there instead.

meain · 2023-10-17T10:52:49Z

src/pkg/control/restore.go

+	// SubService specifies the sub-service which we are restoring in
+	// case of services that are constructed out of multiple services
+	// like Groups.
+	SubService RestoreConfigSubService


RestoreConfig change here.

meain · 2023-10-19T09:24:04Z

src/internal/m365/controller.go

@@ -197,88 +198,6 @@ func (ctrl *Controller) CacheItemInfo(dii details.ItemInfo) {
 	}
 }

-// ---------------------------------------------------------------------------


Move as is to src/internal/m365/service/common/lookup.go.

meain · 2023-10-19T09:25:30Z

src/internal/operations/test/helper_test.go

+
+	var protectedResource idname.Provider = sel
+
+	// In case of  groups, we use the root site as the restore target


Do we have to make it configurable by caller? I didn't think it was required as this is test code.

ryanfkeepers

Looks functional, but I don't fully agree with the design as it splits ownership across multiple systems (configuration, selection) instead of holding to the current pattern of using selectors to source restore data, which would centralize control within a singular interface.

Separately, this PR has too much going on to easily review. There's a lot of refactoring, plus changes across the cli, operations, and service layers. To review it properly these things need to get split out across several PRs. I'd recommend:

first: change selectors to best accomodate subservice declaration and identification according to an interface (we may want a design doc for this).
second: change the operations and service layers to consume the new selectors interface.
third: expose these changes via the CLI and SDK boundaries.

interpolate large refactors as separate PRs along the way, whenever appropriate.

ryanfkeepers · 2023-10-23T16:35:40Z

src/cli/flags/restore_config.go

-	}
+	fs.StringVar(
+		&ToResourceFV, ToResourceFN, "",
+		"Overrides the protected resource (mailbox, site, user, etc) where data gets restored")


Suggested change

"Overrides the protected resource (mailbox, site, user, etc) where data gets restored")

"Overrides which protected resource (mailbox, site, user, etc) receives the restored data")

ryanfkeepers · 2023-10-23T16:37:45Z

src/cli/utils/groups.go

-		} else if len(opts.WebURL)+len(opts.SiteID) > 1 {
-			return clues.New("only a single site can be selected for restore")
-		}
+	// When restoring the user has to select a single site to


Suggested change

// When restoring the user has to select a single site to

// When restoring, the caller must select a single site to

ryanfkeepers · 2023-10-23T16:38:59Z

src/cli/utils/groups.go

+	}
+
+	if len(opts.WebURL)+len(opts.SiteID) > 1 {
+		return clues.New("only a single site can be selected when processing sub resources")


Suggested change

return clues.New("only a single site can be selected when processing sub resources")

return clues.New("cannot select multiple sites for this operation")

ryanfkeepers · 2023-10-23T16:54:52Z

src/internal/m365/service/common/lookup.go

I'm not confident that this code belongs in the common package, either at the top level or as a subpackage. Is there a strong reason not to keep it in its current location at this time?

ryanfkeepers · 2023-10-23T16:55:56Z

src/internal/m365/restore.go

 	"github.com/alcionai/corso/src/pkg/count"
 	"github.com/alcionai/corso/src/pkg/fault"
 	"github.com/alcionai/corso/src/pkg/path"
 )

+func (ctrl *Controller) GetRestoreResource(


Suggested change

func (ctrl *Controller) GetRestoreResource(

func (ctrl *Controller) GetResource(

ryanfkeepers · 2023-10-23T17:07:48Z

src/cli/utils/restore_config.go

+	SubServiceType path.ServiceType
+	SubService     string


This seems to duplicate the selector's ownership. Moving these out of there and into the restore config opens up the potential for conflict between these props and the selector. Similarly, it might lead to building this data into the selector downstream anyway. Iirc, the selector already has the capacity to specify the secondary resource; seems like this identification could be owned there instead.

ashmrtn · 2023-10-27T23:21:39Z

src/internal/operations/restore.go

@@ -227,7 +227,11 @@ func (op *RestoreOperation) do(
 		return nil, clues.Wrap(err, "getting backup and details")
 	}

-	restoreToProtectedResource, err := chooseRestoreResource(ctx, op.rc, op.RestoreCfg, bup.Selector)
+	restoreService, restoreToProtectedResource, err := op.rc.GetRestoreResource(


is there a way the selector and this service can differ? Could we remove the service return value and continue to use the value from the selector? Seems like it would be less error prone

meain · 2023-11-07T10:23:45Z

Temporarily moving over to implementing exports for email as that has moved up in priority. Will get back to rework this after that.

meain · 2024-02-05T14:38:41Z

Closing this PR. Will create a new PR when revisiting this issue.

meain added restore teams & groups Teams and Groups service and API functionality. labels Oct 16, 2023

meain self-assigned this Oct 16, 2023

meain temporarily deployed to Testing October 16, 2023 13:38 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 16, 2023 13:39 — with GitHub Actions Failure

meain temporarily deployed to Testing October 16, 2023 13:39 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 16, 2023 13:39 — with GitHub Actions Failure

meain force-pushed the group-alternate-site branch from 9e62337 to 4ffa2d4 Compare October 16, 2023 14:44

meain temporarily deployed to Testing October 16, 2023 14:44 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 16, 2023 14:45 — with GitHub Actions Failure

meain temporarily deployed to Testing October 16, 2023 14:45 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 16, 2023 14:45 — with GitHub Actions Failure

meain temporarily deployed to Testing October 16, 2023 14:45 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 16, 2023 14:45 — with GitHub Actions Failure

meain force-pushed the group-alternate-site branch from 4ffa2d4 to b70e20d Compare October 17, 2023 09:25

meain temporarily deployed to Testing October 17, 2023 09:26 — with GitHub Actions Inactive

meain temporarily deployed to Testing October 17, 2023 09:27 — with GitHub Actions Inactive

meain had a problem deploying to Testing October 17, 2023 09:27 — with GitHub Actions Failure

meain added 10 commits October 18, 2023 15:26

Fix restoring to different resource

34caca8

Fix existing tests

04d3e90

Update tests for GetRestoreResource

064c120

Update changelog and known issues

ec06158

Fix failing tests

b592ef4

Fix MakeGroupsOpts

ad9c348

Fix few typos

f64e2fe

Fix failing nightly test

9a647aa

Fix runDriveRestoreWithAdvancedOptions

4a2dc77

Fix incremetnal backup tests for Groups

04c3d20

meain force-pushed the group-alternate-site branch from db4853b to 04c3d20 Compare October 18, 2023 11:43

meain temporarily deployed to Testing October 18, 2023 11:44 — with GitHub Actions Inactive

meain temporarily deployed to Testing October 18, 2023 11:45 — with GitHub Actions Inactive

meain commented Oct 19, 2023

View reviewed changes

meain changed the title ~~[WIP] Allow restore data from sites in groups backup to different site~~ Allow restore data from sites in groups backup to different site Oct 19, 2023

meain requested review from ryanfkeepers and ashmrtn October 19, 2023 09:26

meain marked this pull request as ready for review October 19, 2023 09:26

ryanfkeepers suggested changes Oct 23, 2023

View reviewed changes

ashmrtn reviewed Oct 27, 2023

View reviewed changes

meain closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow restore data from sites in groups backup to different site #4509

Allow restore data from sites in groups backup to different site #4509

meain commented Oct 16, 2023 •

edited

Loading

aviator-app bot commented Oct 16, 2023 •

edited

Loading

sonarqubecloud bot commented Oct 16, 2023

sonarqubecloud bot commented Oct 18, 2023

meain Oct 17, 2023

ryanfkeepers Oct 23, 2023

meain Oct 17, 2023

meain Oct 19, 2023

meain Oct 19, 2023

ryanfkeepers left a comment

ryanfkeepers Oct 23, 2023

ryanfkeepers Oct 23, 2023

ryanfkeepers Oct 23, 2023

ryanfkeepers Oct 23, 2023

ryanfkeepers Oct 23, 2023

ryanfkeepers Oct 23, 2023

ashmrtn Oct 27, 2023

meain commented Nov 7, 2023

meain commented Feb 5, 2024


		var protectedResource idname.Provider = sel

		// In case of groups, we use the root site as the restore target

	"Overrides the protected resource (mailbox, site, user, etc) where data gets restored")
	"Overrides which protected resource (mailbox, site, user, etc) receives the restored data")

	// When restoring the user has to select a single site to
	// When restoring, the caller must select a single site to

	return clues.New("only a single site can be selected when processing sub resources")
	return clues.New("cannot select multiple sites for this operation")

	func (ctrl *Controller) GetRestoreResource(
	func (ctrl *Controller) GetResource(

Allow restore data from sites in groups backup to different site #4509

Allow restore data from sites in groups backup to different site #4509

Conversation

meain commented Oct 16, 2023 • edited Loading

Does this PR need a docs update or release note?

Type of change

Issue(s)

Test Plan

aviator-app bot commented Oct 16, 2023 • edited Loading

Current Aviator status

sonarqubecloud bot commented Oct 16, 2023

sonarqubecloud bot commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanfkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meain commented Nov 7, 2023

meain commented Feb 5, 2024

meain commented Oct 16, 2023 •

edited

Loading

aviator-app bot commented Oct 16, 2023 •

edited

Loading