From 391ec21a9a8d51e9246abe7ab14c4a6f0504e69a Mon Sep 17 00:00:00 2001 From: Ryan Diers Date: Tue, 9 May 2023 15:54:41 -0700 Subject: [PATCH] PR feedback and comments --- internal/cli/BUILD.bazel | 1 + internal/cli/helpers.go | 12 ++------- internal/cli/helpers_test.go | 38 ++++++++++++---------------- internal/cli/init.go | 36 +++++++++++++++++++++------ internal/cli/init_test.go | 45 ++++++++++++++++++++++++++++++++++ internal/cli/loader_test.go | 4 ++- internal/cli/retrieve.go | 31 +++++++++++++++++++---- internal/cli/retrieve_test.go | 4 +-- internal/cli/root.go | 37 ++++++++++++++++++++++------ internal/cli/root_test.go | 4 +-- internal/cli/upload.go | 31 +++++++++++++++++++---- internal/cli/upload_test.go | 4 +-- internal/config/config.go | 2 +- internal/config/config_test.go | 4 +-- internal/stores/s3.go | 2 +- 15 files changed, 187 insertions(+), 68 deletions(-) create mode 100644 internal/cli/init_test.go diff --git a/internal/cli/BUILD.bazel b/internal/cli/BUILD.bazel index 8c1ea7a..26e7699 100644 --- a/internal/cli/BUILD.bazel +++ b/internal/cli/BUILD.bazel @@ -27,6 +27,7 @@ go_test( name = "cli_test", srcs = [ "helpers_test.go", + "init_test.go", "loader_test.go", "retrieve_test.go", "root_test.go", diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index b45751c..5c1108e 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log" - "os" "strings" "github.com/discentem/pantri_but_go/internal/config" @@ -13,14 +12,7 @@ import ( "github.com/spf13/afero" ) -func removePathPrefix(objects []string) ([]string, error) { - // Our current path is the prefix to remove as pantri can only be run from the root - // of the repo - pathPrefix, err := os.Getwd() - if err != nil { - return nil, err - } - +func removePathPrefix(pathPrefix string, objects []string) ([]string, error) { for i, object := range objects { objects[i] = strings.TrimPrefix(object, fmt.Sprintf("%s/", pathPrefix)) } @@ -28,7 +20,7 @@ func removePathPrefix(objects []string) ([]string, error) { return objects, nil } -func buildStoresFromConfig(ctx context.Context, cfg config.Config, fsys afero.Fs, opts stores.Options) (stores.Store, error) { +func initStoreFromConfig(ctx context.Context, cfg config.Config, fsys afero.Fs, opts stores.Options) (stores.Store, error) { var s stores.Store switch cfg.StoreType { case stores.StoreTypeS3: diff --git a/internal/cli/helpers_test.go b/internal/cli/helpers_test.go index ac41973..94674b9 100644 --- a/internal/cli/helpers_test.go +++ b/internal/cli/helpers_test.go @@ -14,20 +14,22 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_removePathPrefix(t *testing.T) { +func TestRemovePathPrefix(t *testing.T) { pathPrefix, err := os.Getwd() assert.NoError(t, err) expectedRemovePathPrefixes := []string{"foo/foo.dmg", "bar/bar.pkg"} - testRemovePathPrefixes, err := removePathPrefix([]string{ - fmt.Sprintf("%s/%s", pathPrefix, "foo/foo.dmg"), - fmt.Sprintf("%s/%s", pathPrefix, "bar/bar.pkg"), - }) + testRemovePathPrefixes, err := removePathPrefix( + pathPrefix, + []string{ + fmt.Sprintf("%s/%s", pathPrefix, "foo/foo.dmg"), + fmt.Sprintf("%s/%s", pathPrefix, "bar/bar.pkg"), + }) assert.NoError(t, err) assert.Equal(t, expectedRemovePathPrefixes, testRemovePathPrefixes) } -func Test_buildStoresFromConfig(t *testing.T) { +func TestInitStoreFromConfig(t *testing.T) { ctx := context.Background() cfg := config.Config{ StoreType: stores.StoreTypeS3, @@ -40,7 +42,7 @@ func Test_buildStoresFromConfig(t *testing.T) { fsys := afero.NewMemMapFs() - s, err := buildStoresFromConfig( + s, err := initStoreFromConfig( ctx, cfg, fsys, @@ -61,7 +63,7 @@ func Test_buildStoresFromConfig(t *testing.T) { ) } -func Test_buildStoresFromConfig_Improper_S3Client(t *testing.T) { +func TestInitStoreFromConfig_InvalidOptions(t *testing.T) { ctx := context.Background() cfg := config.Config{ StoreType: stores.StoreTypeS3, @@ -74,22 +76,17 @@ func Test_buildStoresFromConfig_Improper_S3Client(t *testing.T) { fsys := afero.NewMemMapFs() - _, err := buildStoresFromConfig( + _, err := initStoreFromConfig( ctx, cfg, fsys, cfg.Options, ) - t.Log(err.Error()) - - assert.ErrorContains(t, - err, - `improper stores.S3Client init: pantriAddress did not contain s3://, http://, or https:// prefix`, - ) + assert.Errorf(t, err, `improper stores.S3Client init: pantriAddress did not contain s3://, http://, or https:// prefix`) } -func Test_buildStoresFromConfig_ImproperStoreType(t *testing.T) { +func TestInitStoreFromConfig_InvalidateStoreType(t *testing.T) { ctx := context.Background() cfg := config.Config{ StoreType: "s4", @@ -102,17 +99,12 @@ func Test_buildStoresFromConfig_ImproperStoreType(t *testing.T) { fsys := afero.NewMemMapFs() - _, err := buildStoresFromConfig( + _, err := initStoreFromConfig( ctx, cfg, fsys, cfg.Options, ) - t.Log(err.Error()) - - assert.ErrorContains(t, - err, - fmt.Sprintf("type %s is not supported", "s4"), - ) + assert.Errorf(t, err, "type %s is not supported", "s4") } diff --git a/internal/cli/init.go b/internal/cli/init.go index b2bb756..cd2c082 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -10,14 +10,14 @@ import ( "github.com/spf13/viper" ) -func initCommand() *cobra.Command { +func initCmd() *cobra.Command { initCmd := &cobra.Command{ Use: "init", Short: "Initialize a new Pantri repo", Long: "Initialize a new Pantri repo", Args: cobra.ExactArgs(1), - PreRunE: initPreRunE, - RunE: initRunE, + PreRunE: initPreExecFn, + RunE: initFn, } initCmd.PersistentFlags().String("backend_address", "", "Address for the storage backend") @@ -27,8 +27,19 @@ func initCommand() *cobra.Command { return initCmd } -// Bind all the flags to a viper setting so we can use viper everywhere without thinking about it -func initPreRunE(cmd *cobra.Command, args []string) error { +// initPreExecFn is the pre-execution runtime for the initCmd functionality +// in Cobra, PreRunE is a concept of Cobra. It runs before RunE, similar to init() running before +/* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() [X] + // * RunE() + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. +*/ +func initPreExecFn(cmd *cobra.Command, args []string) error { + // Bind all the flags to a viper setting so we can use viper everywhere without thinking about it if err := viper.BindPFlag("backend_address", cmd.PersistentFlags().Lookup("backend_address")); err != nil { return errors.New("Failed to bind backend_address to viper") } @@ -42,7 +53,18 @@ func initPreRunE(cmd *cobra.Command, args []string) error { return nil } -func initRunE(cmd *cobra.Command, args []string) error { +// initFn is the execution runtime for the initCmd functionality +// in Cobra, this is the RunE phase +/* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() [] + // * RunE() [X] + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. +*/ +func initFn(cmd *cobra.Command, args []string) error { repoToInit := args[0] backend := viper.GetString("store_type") fileExt := viper.GetString("metadata_file_extension") @@ -59,7 +81,7 @@ func initRunE(cmd *cobra.Command, args []string) error { switch stores.StoreType(backend) { case stores.StoreTypeS3: - cfg = config.InitializeStoreTypeS3Config( + cfg = config.InitializeStoreTypeS3( cmd.Context(), fsys, repoToInit, diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go new file mode 100644 index 0000000..c59a328 --- /dev/null +++ b/internal/cli/init_test.go @@ -0,0 +1,45 @@ +package cli + +import ( + "testing" + + "github.com/gonuts/go-shellquote" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInitCmd(t *testing.T) { + expectedInitCmdArgs := `init ~/some/git/repo --backend_address http://127.0.0.1:9000/test --store_type=s3 --region="us-east-1"` + + initCmd := initCmd() + + // Split the args and handle bash escape characters + args, err := shellquote.Split(expectedInitCmdArgs) + require.NoError(t, err) + + // Traverse splits the beginning command and args into separate parts + subCmd, subArgs, err := initCmd.Traverse(args) + require.NoError(t, err) + assert.NotNil(t, subCmd) + assert.Equal(t, subCmd.UseLine(), "init") + + // Test the the subArgs equal the expected expectedRetrieveCmdArgs and flags + assert.NoError(t, subCmd.ParseFlags(subArgs)) + + // Test that subArgs expects the same args above + assert.Equal(t, []string{ + "init", + "~/some/git/repo", + "--backend_address", + "http://127.0.0.1:9000/test", + "--store_type=s3", + "--region=us-east-1", + }, + subArgs, + ) + + // Check the required flags exist: + assert.True(t, subCmd.PersistentFlags().Lookup("backend_address").Changed) + assert.True(t, subCmd.PersistentFlags().Lookup("store_type").Changed) + assert.True(t, subCmd.PersistentFlags().Lookup("region").Changed) +} diff --git a/internal/cli/loader_test.go b/internal/cli/loader_test.go index a735025..10f7187 100644 --- a/internal/cli/loader_test.go +++ b/internal/cli/loader_test.go @@ -9,8 +9,10 @@ import ( "github.com/stretchr/testify/require" ) +// TestLoadConfig creates a pantri config file in memory +// to be read and parsed by viper // Test inspired from https://github.com/spf13/viper/blob/master/viper_test.go -func Test_loadConfig(t *testing.T) { +func TestLoadConfig(t *testing.T) { fs := afero.NewMemMapFs() err := fs.Mkdir(".pantri", 0o777) diff --git a/internal/cli/retrieve.go b/internal/cli/retrieve.go index 40c3fe9..4a77717 100644 --- a/internal/cli/retrieve.go +++ b/internal/cli/retrieve.go @@ -6,26 +6,47 @@ import ( "github.com/spf13/cobra" ) -func retrieveCommand() *cobra.Command { +func retrieveCmd() *cobra.Command { retrieveCmd := &cobra.Command{ Use: "retrieve", Short: "retrieve a file from pantri", Long: "retrieve a file from pantri", Args: cobra.MinimumNArgs(1), - // Load the config with OsFs + // PersistentPreRunE + // Loads the config with OsFs + /* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() [X] + // * RunE() + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. + */ PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return loadConfig(afero.NewOsFs()) }, - RunE: retrieveRunE, + RunE: retrieveFn, } return retrieveCmd } -func retrieveRunE(cmd *cobra.Command, objects []string) error { +// retrieveFn is the execution runtime for the retrieveCmd functionality +// in Cobra, this is the RunE phase +/* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() + // * RunE() [X] + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. +*/ +func retrieveFn(cmd *cobra.Command, objects []string) error { fsys := afero.NewOsFs() - s, err := buildStoresFromConfig( + s, err := initStoreFromConfig( cmd.Context(), cfg, fsys, diff --git a/internal/cli/retrieve_test.go b/internal/cli/retrieve_test.go index 31ac14d..c81d594 100644 --- a/internal/cli/retrieve_test.go +++ b/internal/cli/retrieve_test.go @@ -8,10 +8,10 @@ import ( "github.com/stretchr/testify/require" ) -func TestRetrieveCommand(t *testing.T) { +func TestRetrieveCmd(t *testing.T) { expectedRetrieveCmdArgs := "retrieve ./test_file_one ./test_file_two" - retrieveCmd := retrieveCommand() + retrieveCmd := retrieveCmd() // Split the args and handle bash escape characters args, err := shellquote.Split(expectedRetrieveCmdArgs) diff --git a/internal/cli/root.go b/internal/cli/root.go index e10de35..15c5f48 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -21,18 +21,39 @@ var ( ) func ExecuteWithContext(ctx context.Context) error { - return rootCommand().ExecuteContext(ctx) + return rootCmd().ExecuteContext(ctx) } -func rootCommand() *cobra.Command { +func rootCmd() *cobra.Command { rootCmd := &cobra.Command{ Use: "pantri", Short: "A source control friendly binary storage system", Long: "A source control friendly binary storage system", - PreRun: func(cmd *cobra.Command, args []string) { - // set global logger opts + // PersistentPreRun -- all downstream cmds will inherit this fn() + // Set the global logger opts + /* + // The *Run functions are executed in the following order: + // * PersistentPreRun() [X] + // * PreRun() + // * Run() + // * PostRun() + // * PersistentPostRun() + // All functions get the same args, the arguments after the command name. + */ + PersistentPreRun: func(cmd *cobra.Command, args []string) { setLoggerOpts() }, + // RunE + // Return the help page if an error occurs + /* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() + // * RunE() [X] + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. + */ RunE: func(cmd *cobra.Command, args []string) error { if err := cmd.Help(); err != nil { return err @@ -43,6 +64,7 @@ func rootCommand() *cobra.Command { Version: version, } + // At the rootCmd level, set these global flags that will be available to downstream cmds rootCmd.PersistentFlags().BoolVar(&debug, "debug", false, "Run in debug mode") rootCmd.PersistentFlags().BoolVar(&vv, "vv", false, "Run in verbose logging mode") @@ -50,10 +72,11 @@ func rootCommand() *cobra.Command { viper.SetDefault("store_type", stores.StoreTypeUndefined) viper.SetDefault("metadata_file_extension", metadata.MetaDataFileExtension) + // Import subCmds into the rootCmd rootCmd.AddCommand( - initCommand(), - retrieveCommand(), - uploadCommand(), + initCmd(), + retrieveCmd(), + uploadCmd(), ) return rootCmd diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index ba168c6..0e90ad4 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestRootCommand(t *testing.T) { +func TestRootCmd(t *testing.T) { tests := []string{ "pantri", "pantri init", @@ -16,7 +16,7 @@ func TestRootCommand(t *testing.T) { "pantri retrieve", } - rootCmd := rootCommand() + rootCmd := rootCmd() for _, tc := range tests { tc := tc diff --git a/internal/cli/upload.go b/internal/cli/upload.go index bd81e8a..4e23b5a 100644 --- a/internal/cli/upload.go +++ b/internal/cli/upload.go @@ -6,26 +6,47 @@ import ( "github.com/spf13/cobra" ) -func uploadCommand() *cobra.Command { +func uploadCmd() *cobra.Command { uploadCmd := &cobra.Command{ Use: "upload", Short: "Upload a file to pantri", Long: "Upload a file to pantri", Args: cobra.MinimumNArgs(1), - // Load the config with OsFs + // PersistentPreRunE + // Loads the config with OsFs + /* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() [X] + // * RunE() + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. + */ PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return loadConfig(afero.NewOsFs()) }, - RunE: uploadRunE, + RunE: uploadFn, } return uploadCmd } -func uploadRunE(cmd *cobra.Command, objects []string) error { +// uploadFn is the execution runtime for the uploadCmd functionality +// in Cobra, this is the RunE phase +/* + // The *Run functions are executed in the following order: + // * PersistentPreRunE() + // * PreRunE() + // * RunE() [X] + // * PostRunE() + // * PersistentPostRunE() + // All functions get the same args, the arguments after the command name. +*/ +func uploadFn(cmd *cobra.Command, objects []string) error { fsys := afero.NewOsFs() - s, err := buildStoresFromConfig( + s, err := initStoreFromConfig( cmd.Context(), cfg, fsys, diff --git a/internal/cli/upload_test.go b/internal/cli/upload_test.go index 3b9d4d0..bf201a5 100644 --- a/internal/cli/upload_test.go +++ b/internal/cli/upload_test.go @@ -8,10 +8,10 @@ import ( "github.com/stretchr/testify/require" ) -func TestUploadCommand(t *testing.T) { +func TestUploadCmd(t *testing.T) { expectedUploadCmdArgs := "upload ./test_file_one ./test_file_two" - uploadCmd := uploadCommand() + uploadCmd := uploadCmd() // Split the args and handle bash escape characters args, err := shellquote.Split(expectedUploadCmdArgs) diff --git a/internal/config/config.go b/internal/config/config.go index 700838f..e89b7bc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,7 +34,7 @@ var ( ErrConfigDirNotExist = errors.New("config directory does not exist") ) -func InitializeStoreTypeS3Config( +func InitializeStoreTypeS3( ctx context.Context, fsys afero.Fs, sourceRepo, pantriAddress, awsRegion string, diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c82b481..467921f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -81,7 +81,7 @@ func TestWrite(t *testing.T) { t.Run("successful write", TestSuccessfulWrite) } -func TestInitializeStoreTypeS3Config(t *testing.T) { +func TestInitializeStoreTypeS3(t *testing.T) { ctx := context.Background() fsys := afero.NewMemMapFs() @@ -91,7 +91,7 @@ func TestInitializeStoreTypeS3Config(t *testing.T) { Region: "us-east-9876", } - cfg := InitializeStoreTypeS3Config( + cfg := InitializeStoreTypeS3( ctx, fsys, "~/some_repo_root", diff --git a/internal/stores/s3.go b/internal/stores/s3.go index c2a9fcb..c885577 100644 --- a/internal/stores/s3.go +++ b/internal/stores/s3.go @@ -27,7 +27,7 @@ type S3Store struct { Options Options `json:"options" mapstructure:"options"` fsys afero.Fs awsRegion string - // Migrate to internal/s3Client instead of using s3Client directly from AWS_SDK + // TODO (@radsec) - extract this S3 logic to a separate internal client instead of directly from AWS_SDK s3Client *s3.Client s3Uploader *s3manager.Uploader s3Downloader *s3manager.Downloader