PMM-4879: defaults-file param #171

ritbl · 2021-12-23T10:28:35Z

provides extension point to process default params by implementing commands.ApplyDefaults
username and password set via CLI flag have higher priority (e.g. values from defaults file are used only if values are not initialized)

# Conflicts: # go.mod # go.sum

ritbl · 2021-12-23T11:16:53Z

commands/base_test.go

@@ -110,3 +113,63 @@ func TestReadFile(t *testing.T) {
 		require.Empty(t, certificate)
 	})
 }
+
+type cmdWithDefaultsApply struct {
+	applyDefaultCalled bool


used for tests only

why don't use mock libraries?

github-actions · 2021-12-23T11:17:23Z

commands/management/add_mysql_test.go

+
+		cmd := &addMySQLCommand{}
+
+		commands.ConfigureDefaults(file.Name(), cmd)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Error return value of commands.ConfigureDefaults is not checked (errcheck)

github-actions · 2021-12-23T11:17:23Z

commands/management/add_mysql_test.go

+			Password: "default-password",
+		}
+
+		commands.ConfigureDefaults(file.Name(), cmd)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Error return value of commands.ConfigureDefaults is not checked (errcheck)

github-actions · 2021-12-23T11:17:23Z

commands/management/add_mysql_test.go

+			Password: "default-password",
+		}
+
+		commands.ConfigureDefaults(file.Name(), cmd)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Error return value of commands.ConfigureDefaults is not checked (errcheck)

github-actions · 2021-12-23T11:17:23Z

commands/management/add_mysql_test.go

@@ -160,3 +162,92 @@ func TestRun(t *testing.T) {
 		}
 	})
 }
+
+func TestApplyDefaults(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function 'TestApplyDefaults' is too long (86 > 60) (funlen)

github-actions · 2021-12-23T11:17:23Z

commands/base.go

@@ -69,6 +73,10 @@ type Command interface {
 	Run() (Result, error)
 }

+type ApplyDefaults interface {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
exported type ApplyDefaults should have comment or be unexported (golint)

github-actions · 2021-12-23T11:17:24Z

commands/base_test.go

+}
+
+func TestConfigureDefaults(t *testing.T) {
+	t.Run("ApplyDefaults is called if command supports it", func(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestConfigureDefaults has missing the call to method parallel in the test run (paralleltest)

github-actions · 2021-12-23T11:17:24Z

commands/base_test.go

+		assert.Equal(t, "toor", cmd.password)
+	})
+
+	t.Run("ApplyDefaults is not called if pass is not setup", func(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestConfigureDefaults has missing the call to method parallel in the test run (paralleltest)

github-actions · 2021-12-23T11:17:24Z

commands/base_test.go

+	})
+}
+
+func TestExpandPath(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestExpandPath missing the call to method parallel (paralleltest)

github-actions · 2021-12-23T11:17:24Z

commands/base_test.go

+}
+
+func TestExpandPath(t *testing.T) {
+	t.Run("relative to userhome", func(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestExpandPath has missing the call to method parallel in the test run (paralleltest)

github-actions · 2021-12-23T11:17:24Z

commands/base_test.go

+
+		assert.Equal(t, usr.HomeDir, actual)
+	})
+	t.Run("relative to userhome", func(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestExpandPath has missing the call to method parallel in the test run (paralleltest)

ritbl · 2021-12-23T11:18:37Z

commands/base_test.go

+
+func TestConfigureDefaults(t *testing.T) {
+	t.Run("ApplyDefaults is called if command supports it", func(t *testing.T) {
+		file, e := os.Open("../testdata/.my.cnf")


@BupycHuk you were asking for it. I use real file in cmd test, but tmp files for commands/management/add_mysql.go

github-actions · 2021-12-23T12:16:13Z

commands/management/add_mysql_test.go

@@ -160,3 +164,87 @@ func TestRun(t *testing.T) {
 		}
 	})
 }
+
+func TestApplyDefaults(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function 'TestApplyDefaults' is too long (81 > 60) (funlen)

ritbl · 2021-12-23T12:22:24Z

testdata/.my.cnf

@@ -0,0 +1,5 @@
+[client]


based on https://dev.mysql.com/doc/refman/8.0/en/mysql-config-editor.html

BupycHuk · 2022-01-18T22:52:05Z

commands/base.go

@@ -184,6 +193,37 @@ func (e errFromNginx) GoString() string {
 	return fmt.Sprintf("errFromNginx(%q)", string(e))
 }

+func ConfigureDefaults(config string, cmd ApplyDefaults) error {


i think its better to rename it to configPath

BupycHuk · 2022-01-18T22:54:24Z

commands/base_test.go

 	"strings"
 	"testing"

+	"gopkg.in/ini.v1"
+


let's remove this blank line

BupycHuk · 2022-01-18T22:55:07Z

commands/base_test.go

@@ -110,3 +113,63 @@ func TestReadFile(t *testing.T) {
 		require.Empty(t, certificate)
 	})
 }
+
+type cmdWithDefaultsApply struct {
+	applyDefaultCalled bool


why don't use mock libraries?

BupycHuk · 2022-01-18T23:10:16Z

commands/base_test.go

+
+		assert.Equal(t, usr.HomeDir, actual)
+	})
+	t.Run("relative to userhome", func(t *testing.T) {


it doesn't look relative to userhome

BupycHuk · 2022-01-18T23:14:56Z

commands/management/add_mysql.go

+	"github.com/sirupsen/logrus"
+
+	"gopkg.in/ini.v1"
+
+	"github.com/percona/pmm-admin/agentlocal"
+


please fix imports to split them in 3 blocks

go libs

3rd party libs

local packages

here and in other files

ritbl added 3 commits October 17, 2021 23:42

PMM-4879: introduce default-file option and ApplyDefaults hook

b85b140

PMM-4879: set username and password from defaults for mysqlAdd

16b5d40

PMM-4879 refactor

c97ebdd

ritbl requested review from percona-csalguero, BupycHuk and JiriCtvrtka December 23, 2021 10:28

ritbl added 7 commits December 23, 2021 11:29

PMM-4879: refactor

fa676c4

PMM-4879: introduce default-file option and ApplyDefaults hook

019efcf

PMM-4879: set username and password from defaults for mysqlAdd

c872a6e

PMM-4879 refactor

49cfcf3

PMM-4879: refactor

95f50de

PMM-4879: mod tidy

c261e2f

Merge remote-tracking branch 'percona/PMM-4879' into PMM-4879

fca7544

# Conflicts: # go.mod # go.sum

percona-csalguero approved these changes Dec 23, 2021

View reviewed changes

PMM-4879: cleanup

5e0abde

ritbl commented Dec 23, 2021

View reviewed changes

github-actions bot reviewed Dec 23, 2021

View reviewed changes

ritbl commented Dec 23, 2021

View reviewed changes

ritbl added 2 commits December 23, 2021 13:10

PMM-4879: cleanup

46da034

PMM-4879: style

e4188c2

github-actions bot reviewed Dec 23, 2021

View reviewed changes

ritbl marked this pull request as ready for review December 23, 2021 12:19

ritbl commented Dec 23, 2021

View reviewed changes

testdata/.my.cnf

@@ -0,0 +1,5 @@

[client]

Copy link

Contributor Author

ritbl Dec 23, 2021

Choose a reason for hiding this comment

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

based on https://dev.mysql.com/doc/refman/8.0/en/mysql-config-editor.html

Merge branch 'main' into PMM-4879

55a1a43

JiriCtvrtka approved these changes Dec 30, 2021

View reviewed changes

Merge branch 'main' into PMM-4879

14a8539

BupycHuk suggested changes Jan 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMM-4879: defaults-file param #171

PMM-4879: defaults-file param #171

ritbl commented Dec 23, 2021 •

edited

Loading

ritbl Dec 23, 2021

BupycHuk Jan 18, 2022

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

github-actions bot Dec 23, 2021

ritbl Dec 23, 2021

github-actions bot Dec 23, 2021

ritbl Dec 23, 2021

BupycHuk Jan 18, 2022

BupycHuk Jan 18, 2022

BupycHuk Jan 18, 2022

BupycHuk Jan 18, 2022

BupycHuk Jan 18, 2022

BupycHuk Jan 18, 2022


		cmd := &addMySQLCommand{}

		commands.ConfigureDefaults(file.Name(), cmd)

PMM-4879: defaults-file param #171

Are you sure you want to change the base?

PMM-4879: defaults-file param #171

Conversation

ritbl commented Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Dec 23, 2021

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

ritbl commented Dec 23, 2021 •

edited

Loading