From 8a2e63dcadd0e2cf94a5f3052ce935c04dfe21ed Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Tue, 25 Apr 2023 11:35:50 -0700 Subject: [PATCH] CLI connect refactor (#3213) Move code for connecting to a repo into a common package so that backup and restore CLI code can both use it This will also make it easier for maintenance code in the future as it can reuse the same helper There are no logic changes in this PR, only code movement --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) tangentially related to * #3077 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/cli/backup/backup.go | 21 ++------------------- src/cli/backup/exchange.go | 4 ++-- src/cli/backup/onedrive.go | 4 ++-- src/cli/backup/sharepoint.go | 4 ++-- src/cli/config/account.go | 3 +-- src/cli/config/config.go | 12 ++++++++++++ src/cli/config/config_test.go | 21 +++++++++++++++++++++ src/cli/config/storage.go | 3 +-- src/cli/restore/exchange.go | 9 +-------- src/cli/restore/onedrive.go | 9 +-------- src/cli/restore/sharepoint.go | 9 +-------- src/cli/utils/utils.go | 21 +++++++++++++-------- src/cli/utils/utils_test.go | 22 ---------------------- 13 files changed, 59 insertions(+), 83 deletions(-) diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index 7c9e39dd0..fe2a07a75 100644 --- a/src/cli/backup/backup.go +++ b/src/cli/backup/backup.go @@ -9,13 +9,10 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" - "github.com/alcionai/corso/src/cli/config" - "github.com/alcionai/corso/src/cli/options" . "github.com/alcionai/corso/src/cli/print" "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/data" - "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/backup" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" @@ -270,7 +267,7 @@ func genericDeleteCommand(cmd *cobra.Command, bID, designation string, args []st ctx := clues.Add(cmd.Context(), "delete_backup_id", bID) - r, _, err := getAccountAndConnect(ctx) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } @@ -291,7 +288,7 @@ func genericDeleteCommand(cmd *cobra.Command, bID, designation string, args []st func genericListCommand(cmd *cobra.Command, bID string, service path.ServiceType, args []string) error { ctx := cmd.Context() - r, _, err := getAccountAndConnect(ctx) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } @@ -324,20 +321,6 @@ func genericListCommand(cmd *cobra.Command, bID string, service path.ServiceType return nil } -func getAccountAndConnect(ctx context.Context) (repository.Repository, *account.Account, error) { - cfg, err := config.GetConfigRepoDetails(ctx, true, nil) - if err != nil { - return nil, nil, err - } - - r, err := repository.Connect(ctx, cfg.Account, cfg.Storage, options.Control()) - if err != nil { - return nil, nil, clues.Wrap(err, "Failed to connect to the "+cfg.Storage.Provider.String()+" repository") - } - - return r, &cfg.Account, nil -} - func ifShow(flag string) bool { return strings.ToLower(strings.TrimSpace(flag)) == "show" } diff --git a/src/cli/backup/exchange.go b/src/cli/backup/exchange.go index ee0070e25..b1ad92cb2 100644 --- a/src/cli/backup/exchange.go +++ b/src/cli/backup/exchange.go @@ -153,7 +153,7 @@ func createExchangeCmd(cmd *cobra.Command, args []string) error { return err } - r, acct, err := getAccountAndConnect(ctx) + r, acct, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } @@ -265,7 +265,7 @@ func detailsExchangeCmd(cmd *cobra.Command, args []string) error { ctx := cmd.Context() opts := utils.MakeExchangeOpts(cmd) - r, _, err := getAccountAndConnect(ctx) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } diff --git a/src/cli/backup/onedrive.go b/src/cli/backup/onedrive.go index 006ae087b..27cfeb244 100644 --- a/src/cli/backup/onedrive.go +++ b/src/cli/backup/onedrive.go @@ -135,7 +135,7 @@ func createOneDriveCmd(cmd *cobra.Command, args []string) error { return err } - r, acct, err := getAccountAndConnect(ctx) + r, acct, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } @@ -224,7 +224,7 @@ func detailsOneDriveCmd(cmd *cobra.Command, args []string) error { ctx := cmd.Context() opts := utils.MakeOneDriveOpts(cmd) - r, _, err := getAccountAndConnect(ctx) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } diff --git a/src/cli/backup/sharepoint.go b/src/cli/backup/sharepoint.go index 2b84ffe90..354e4cf9e 100644 --- a/src/cli/backup/sharepoint.go +++ b/src/cli/backup/sharepoint.go @@ -146,7 +146,7 @@ func createSharePointCmd(cmd *cobra.Command, args []string) error { return err } - r, acct, err := getAccountAndConnect(ctx) + r, acct, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } @@ -308,7 +308,7 @@ func detailsSharePointCmd(cmd *cobra.Command, args []string) error { ctx := cmd.Context() opts := utils.MakeSharePointOpts(cmd) - r, _, err := getAccountAndConnect(ctx) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } diff --git a/src/cli/config/account.go b/src/cli/config/account.go index 3bcd9fcd2..310ac97c3 100644 --- a/src/cli/config/account.go +++ b/src/cli/config/account.go @@ -6,7 +6,6 @@ import ( "github.com/alcionai/clues" "github.com/spf13/viper" - "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/credentials" @@ -72,7 +71,7 @@ func configureAccount( } // ensure required properties are present - if err := utils.RequireProps(map[string]string{ + if err := requireProps(map[string]string{ credentials.AzureClientID: m365Cfg.AzureClientID, credentials.AzureClientSecret: m365Cfg.AzureClientSecret, account.AzureTenantID: m365Cfg.AzureTenantID, diff --git a/src/cli/config/config.go b/src/cli/config/config.go index e48f6f5ce..5cdf7863c 100644 --- a/src/cli/config/config.go +++ b/src/cli/config/config.go @@ -321,3 +321,15 @@ func mustMatchConfig(vpr *viper.Viper, m map[string]string) error { return nil } + +// requireProps validates the existence of the properties +// in the map. Expects the format map[propName]propVal. +func requireProps(props map[string]string) error { + for name, val := range props { + if len(val) == 0 { + return clues.New(name + " is required to perform this command") + } + } + + return nil +} diff --git a/src/cli/config/config_test.go b/src/cli/config/config_test.go index d9b2be563..1226902bb 100644 --- a/src/cli/config/config_test.go +++ b/src/cli/config/config_test.go @@ -39,6 +39,27 @@ func TestConfigSuite(t *testing.T) { suite.Run(t, &ConfigSuite{Suite: tester.NewUnitSuite(t)}) } +func (suite *ConfigSuite) TestRequireProps() { + table := []struct { + name string + props map[string]string + errCheck assert.ErrorAssertionFunc + }{ + { + props: map[string]string{"exists": "I have seen the fnords!"}, + errCheck: assert.NoError, + }, + { + props: map[string]string{"not-exists": ""}, + errCheck: assert.Error, + }, + } + for _, test := range table { + err := requireProps(test.props) + test.errCheck(suite.T(), err, clues.ToCore(err)) + } +} + func (suite *ConfigSuite) TestReadRepoConfigBasic() { var ( t = suite.T() diff --git a/src/cli/config/storage.go b/src/cli/config/storage.go index a10c3315e..9aba1e5d9 100644 --- a/src/cli/config/storage.go +++ b/src/cli/config/storage.go @@ -9,7 +9,6 @@ import ( "github.com/aws/aws-sdk-go/aws/defaults" "github.com/spf13/viper" - "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/pkg/credentials" "github.com/alcionai/corso/src/pkg/storage" @@ -112,7 +111,7 @@ func configureStorage( } // ensure required properties are present - if err := utils.RequireProps(map[string]string{ + if err := requireProps(map[string]string{ storage.Bucket: s3Cfg.Bucket, credentials.CorsoPassphrase: corso.CorsoPassphrase, }); err != nil { diff --git a/src/cli/restore/exchange.go b/src/cli/restore/exchange.go index 1fb098531..5e36198aa 100644 --- a/src/cli/restore/exchange.go +++ b/src/cli/restore/exchange.go @@ -6,14 +6,12 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" - "github.com/alcionai/corso/src/cli/config" "github.com/alcionai/corso/src/cli/options" . "github.com/alcionai/corso/src/cli/print" "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/pkg/control" - "github.com/alcionai/corso/src/pkg/repository" ) // called by restore.go to map subcommands to provider-specific handling. @@ -90,16 +88,11 @@ func restoreExchangeCmd(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.GetConfigRepoDetails(ctx, true, nil) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } - r, err := repository.Connect(ctx, cfg.Account, cfg.Storage, options.Control()) - if err != nil { - return Only(ctx, clues.Wrap(err, "Failed to connect to the "+cfg.Storage.Provider.String()+" repository")) - } - defer utils.CloseRepo(ctx, r) dest := control.DefaultRestoreDestination(common.SimpleDateTime) diff --git a/src/cli/restore/onedrive.go b/src/cli/restore/onedrive.go index 6e61e9386..474b5ae6d 100644 --- a/src/cli/restore/onedrive.go +++ b/src/cli/restore/onedrive.go @@ -6,14 +6,12 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" - "github.com/alcionai/corso/src/cli/config" "github.com/alcionai/corso/src/cli/options" . "github.com/alcionai/corso/src/cli/print" "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/pkg/control" - "github.com/alcionai/corso/src/pkg/repository" ) // called by restore.go to map subcommands to provider-specific handling. @@ -92,16 +90,11 @@ func restoreOneDriveCmd(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.GetConfigRepoDetails(ctx, true, nil) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } - r, err := repository.Connect(ctx, cfg.Account, cfg.Storage, options.Control()) - if err != nil { - return Only(ctx, clues.Wrap(err, "Failed to connect to the "+cfg.Storage.Provider.String()+" repository")) - } - defer utils.CloseRepo(ctx, r) dest := control.DefaultRestoreDestination(common.SimpleDateTimeOneDrive) diff --git a/src/cli/restore/sharepoint.go b/src/cli/restore/sharepoint.go index 8c0e5bfb2..4a0ca41bf 100644 --- a/src/cli/restore/sharepoint.go +++ b/src/cli/restore/sharepoint.go @@ -6,14 +6,12 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" - "github.com/alcionai/corso/src/cli/config" "github.com/alcionai/corso/src/cli/options" . "github.com/alcionai/corso/src/cli/print" "github.com/alcionai/corso/src/cli/utils" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/pkg/control" - "github.com/alcionai/corso/src/pkg/repository" ) // called by restore.go to map subcommands to provider-specific handling. @@ -91,16 +89,11 @@ func restoreSharePointCmd(cmd *cobra.Command, args []string) error { return err } - cfg, err := config.GetConfigRepoDetails(ctx, true, nil) + r, _, err := utils.GetAccountAndConnect(ctx) if err != nil { return Only(ctx, err) } - r, err := repository.Connect(ctx, cfg.Account, cfg.Storage, options.Control()) - if err != nil { - return Only(ctx, clues.Wrap(err, "Failed to connect to the "+cfg.Storage.Provider.String()+" repository")) - } - defer utils.CloseRepo(ctx, r) dest := control.DefaultRestoreDestination(common.SimpleDateTimeOneDrive) diff --git a/src/cli/utils/utils.go b/src/cli/utils/utils.go index 6be59d367..b41e7703f 100644 --- a/src/cli/utils/utils.go +++ b/src/cli/utils/utils.go @@ -8,7 +8,10 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "github.com/alcionai/corso/src/cli/config" + "github.com/alcionai/corso/src/cli/options" "github.com/alcionai/corso/src/internal/events" + "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" @@ -21,16 +24,18 @@ const ( Wildcard = "*" ) -// RequireProps validates the existence of the properties -// in the map. Expects the format map[propName]propVal. -func RequireProps(props map[string]string) error { - for name, val := range props { - if len(val) == 0 { - return clues.New(name + " is required to perform this command") - } +func GetAccountAndConnect(ctx context.Context) (repository.Repository, *account.Account, error) { + cfg, err := config.GetConfigRepoDetails(ctx, true, nil) + if err != nil { + return nil, nil, err } - return nil + r, err := repository.Connect(ctx, cfg.Account, cfg.Storage, options.Control()) + if err != nil { + return nil, nil, clues.Wrap(err, "Failed to connect to the "+cfg.Storage.Provider.String()+" repository") + } + + return r, &cfg.Account, nil } // CloseRepo handles closing a repo. diff --git a/src/cli/utils/utils_test.go b/src/cli/utils/utils_test.go index f942e61f3..e6f5340d4 100644 --- a/src/cli/utils/utils_test.go +++ b/src/cli/utils/utils_test.go @@ -3,7 +3,6 @@ package utils import ( "testing" - "github.com/alcionai/clues" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" @@ -19,27 +18,6 @@ func TestCliUtilsSuite(t *testing.T) { suite.Run(t, &CliUtilsSuite{Suite: tester.NewUnitSuite(t)}) } -func (suite *CliUtilsSuite) TestRequireProps() { - table := []struct { - name string - props map[string]string - errCheck assert.ErrorAssertionFunc - }{ - { - props: map[string]string{"exists": "I have seen the fnords!"}, - errCheck: assert.NoError, - }, - { - props: map[string]string{"not-exists": ""}, - errCheck: assert.Error, - }, - } - for _, test := range table { - err := RequireProps(test.props) - test.errCheck(suite.T(), err, clues.ToCore(err)) - } -} - func (suite *CliUtilsSuite) TestSplitFoldersIntoContainsAndPrefix() { table := []struct { name string