From 259736b1692dc43ec61c68c6998c6db673179959 Mon Sep 17 00:00:00 2001 From: Yonghwan SO Date: Sat, 4 Jun 2022 22:48:58 +0900 Subject: [PATCH 1/4] fixed ignored --env flag issue --- soda/cmd/root.go | 16 +++++++++++++++- soda/cmd/root_integration_test.go | 10 ++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/soda/cmd/root.go b/soda/cmd/root.go index 1cc163d3..219540ae 100644 --- a/soda/cmd/root.go +++ b/soda/cmd/root.go @@ -20,8 +20,22 @@ var RootCmd = &cobra.Command{ Short: "A tasty treat for all your database needs", PersistentPreRun: func(c *cobra.Command, args []string) { fmt.Printf("pop %s\n\n", Version) + + /* NOTE: Do not use c.PersistentFlags. `c` is not always the + RootCmd. The naming is confusing. The meaning of "persistent" + in the `PersistentPreRun` is something like "this function will + be 'sticky' to all subcommands and will run for them. So `c` + can be any subcommands. + However, the meaning of "persistent" in the `PersistentFlags` + is, as the function comment said, "persistent FlagSet + specifically set in the **current command**" so it is sticky + to specific command! + + Use c.Flags() or c.Root().Flags() here. + */ + // CLI flag has priority - if !c.PersistentFlags().Changed("env") { + if !c.Flags().Changed("env") { env = defaults.String(os.Getenv("GO_ENV"), env) } // TODO! Only do this when the command needs it. diff --git a/soda/cmd/root_integration_test.go b/soda/cmd/root_integration_test.go index 3a6b40c2..935daf69 100644 --- a/soda/cmd/root_integration_test.go +++ b/soda/cmd/root_integration_test.go @@ -7,20 +7,21 @@ import ( "github.com/stretchr/testify/require" ) -func Test_RootCmd_NoArg(t *testing.T) { +func Test_RootCmd_Environtment(t *testing.T) { oldEnv := os.Getenv("GO_ENV") defer os.Setenv("GO_ENV", oldEnv) - // Fallback on default env r := require.New(t) c := RootCmd - c.SetArgs([]string{}) + + // Fallback on default env + c.SetArgs([]string{"help"}) err := c.Execute() r.NoError(err) r.Equal("development", env) // Override with GO_ENV - c.SetArgs([]string{}) + c.SetArgs([]string{"help"}) os.Setenv("GO_ENV", "test") err = c.Execute() r.NoError(err) @@ -28,6 +29,7 @@ func Test_RootCmd_NoArg(t *testing.T) { // CLI flag priority c.SetArgs([]string{ + "help", "--env", "production", }) From 28b4b462dad28f306499e77e30e0c1af93ccf1fa Mon Sep 17 00:00:00 2001 From: Yonghwan SO Date: Sat, 4 Jun 2022 23:31:37 +0900 Subject: [PATCH 2/4] added comment for the newly exposed function CanonicalDialect --- pop.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pop.go b/pop.go index 97d025a7..3cfeef05 100644 --- a/pop.go +++ b/pop.go @@ -56,6 +56,12 @@ func DialectSupported(d string) bool { return false } +// CanonicalDialect checks if the given synonym (could be a valid dialect too) +// is a registered synonym and returns the canonical dialect for the synonym. +// Otherwise, it returns the lowercase value of the given synonym. +// +// Note that it does not check if the return value is a valid (supported) +// dialect so you need to check it with `DialectSupported()`. func CanonicalDialect(synonym string) string { d := strings.ToLower(synonym) if syn, ok := dialectSynonyms[d]; ok { From d46bf42d211919bfd7919231101b10a75cce054f Mon Sep 17 00:00:00 2001 From: Yonghwan SO Date: Tue, 28 Jun 2022 23:35:07 +0900 Subject: [PATCH 3/4] fixed typo and added one more test case --- soda/cmd/root_integration_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/soda/cmd/root_integration_test.go b/soda/cmd/root_integration_test.go index 935daf69..70291a18 100644 --- a/soda/cmd/root_integration_test.go +++ b/soda/cmd/root_integration_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/require" ) -func Test_RootCmd_Environtment(t *testing.T) { +func Test_RootCmd_Environment(t *testing.T) { oldEnv := os.Getenv("GO_ENV") defer os.Setenv("GO_ENV", oldEnv) @@ -27,6 +27,18 @@ func Test_RootCmd_Environtment(t *testing.T) { r.NoError(err) r.Equal("test", env) + // CLI flag priority: the preferred order of flags and commands + c.SetArgs([]string{ + "--env", + "production", + "help", + }) + os.Setenv("GO_ENV", "test") + err = c.Execute() + r.NoError(err) + r.Equal("production", env) + + // the following order works fine now but need to be considered again // CLI flag priority c.SetArgs([]string{ "help", From 9f829f6094c6f4195bf5ff96cb426097f970b86e Mon Sep 17 00:00:00 2001 From: Yonghwan SO Date: Wed, 29 Jun 2022 00:30:27 +0900 Subject: [PATCH 4/4] removed tests for go1.16 and disabled race detector on windows --- .github/workflows/tests.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d3f9c48e..240ba5c9 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -12,7 +12,6 @@ jobs: strategy: matrix: go-version: - - "1.16.x" - "1.17.x" - "1.18.x" @@ -56,7 +55,6 @@ jobs: strategy: matrix: go-version: - - "1.16.x" - "1.17.x" - "1.18.x" @@ -102,7 +100,6 @@ jobs: strategy: matrix: go-version: - - "1.16.x" - "1.17.x" - "1.18.x" @@ -146,7 +143,6 @@ jobs: strategy: matrix: go-version: - - "1.16.x" - "1.17.x" - "1.18.x" @@ -187,7 +183,6 @@ jobs: strategy: matrix: go-version: - - "1.16.x" - "1.17.x" - "1.18.x" os: @@ -213,7 +208,15 @@ jobs: shell: bash - name: Test + if: ${{ matrix.os != 'windows-latest' }} env: SODA_DIALECT: "sqlite" run: | go test -tags sqlite -race -cover ./... + + - name: Short Test + if: ${{ matrix.os == 'windows-latest' }} + env: + SODA_DIALECT: "sqlite" + run: | + go test -tags sqlite ./...