Repository: datreeio/datree
Pull request #405 (2022-01-30 17:48:06)

feat: print version message as a part of prerune hook
Fixes: #386

**Description**
Run `LoadVersionMessages` as a part of PreRunE hook

**Type of changes**
Refactor/Enhancements

dimabru: @myishay 
can you have a look at this pr?
dimabru: hey @amustaque97,
Thanks for your pr. Our team will review it as soon as possible.
Please note that it might take a few days
noaabarki: Hey @amustaque97. Great! I reviewed your PR and I think there are a few things we can do to make it better :)
- Avoid fetching & printing the messages when `--output=json/XML/YAM`L:
We primarily use LoadVersionMessages to print deprecation and important messages for our users.  At the moment, if the user added the `--output=json/XML/YAML` flag they won’t be printed. As we discussed before in the issue, in the future we’d like to print them as well however, until then we want to keep the current behavior. I suggest wrapping the entire logic for fetching and printing the messages with `if (flags.Output != "json") && (flags.Output != "yaml") && (flags.Output != "xml")` inside the `PreRunE` hook.
- Fetch & Print messages in one function and call it in all commands:
It’s the same logic everywhere and it feels a little redundant. I suggest wrapping it in a `func(ctx...)`. For that, I advise changing the `LoadVersionMessages` signature into `LoadVersionMessages(cliVersion string) chan *VersionMessage` to create the messages channel and return it. (It’s also good practice in Go).
Eventually, this should be like this ->
```
PreRun: func(cmd *cobra.Command, args []string) {
			if (testCommandFlags.Output != "json") && (testCommandFlags.Output != "yaml") && (testCommandFlags.Output != "xml") {
				// this should be in a separate function
				messages := ctx.Messager.LoadVersionMessages(ctx.CliVersion)
				defer func() {
					for msg := range messages {
						ctx.Printer.PrintMessage(msg.MessageText+"\n", msg.MessageColor)
					}
				}()
			}
		}
```
amustaque97: Thank you for the in detail review. Please note: I'm currently occupied with some other work. Might be late to address review comments.
Thank you
noaabarki: Hey @Abhra303, thanks for your PR 👑 We will review your PR soon, please note that it might take a day or two 🙏🏼
noaabarki: LGTM. After changes will be submitted we can merge this PR 🥳

Patch:
diff --git a/bl/messager/messager.go b/bl/messager/messager.go
index 4dfcfcb..9d83ede 100644
--- a/bl/messager/messager.go
+++ b/bl/messager/messager.go
@@ -26,7 +26,8 @@ type VersionMessage struct {
 	MessageColor string
 }
 
-func (m *Messager) LoadVersionMessages(messages chan *VersionMessage, cliVersion string) {
+func (m *Messager) LoadVersionMessages(cliVersion string) chan *VersionMessage {
+	messages := make(chan *VersionMessage, 1)
 	go func() {
 		msg, _ := m.messagesClient.GetVersionMessage(cliVersion, 900)
 		if msg != nil {
@@ -34,6 +35,7 @@ func (m *Messager) LoadVersionMessages(messages chan *VersionMessage, cliVersion
 		}
 		close(messages)
 	}()
+	return messages
 }
 
 func (m *Messager) toVersionMessage(msg *cliClient.VersionMessage) *VersionMessage {
diff --git a/cmd/config/main.go b/cmd/config/main.go
index ee2910a..c3e5e87 100644
--- a/cmd/config/main.go
+++ b/cmd/config/main.go
@@ -8,7 +8,7 @@ import (
 )
 
 type Messager interface {
-	LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string)
+	LoadVersionMessages(cliVersion string) chan *messager.VersionMessage
 }
 
 type Printer interface {
diff --git a/cmd/config/set.go b/cmd/config/set.go
index 59ccd71..fc9967f 100644
--- a/cmd/config/set.go
+++ b/cmd/config/set.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"reflect"
 
-	"github.com/datreeio/datree/bl/messager"
 	"github.com/spf13/cobra"
 )
 
@@ -15,8 +14,7 @@ func NewSetCommand(ctx *ConfigCommandContext) *cobra.Command {
 		Short: "Set configuration value",
 		Long:  `Apply value for specific key in datree config.yaml file. Defaults to $HOME/.datree/config.yaml`,
 		Run: func(cmd *cobra.Command, args []string) {
-			messages := make(chan *messager.VersionMessage, 1)
-			go ctx.Messager.LoadVersionMessages(messages, ctx.CliVersion)
+			messages := ctx.Messager.LoadVersionMessages(ctx.CliVersion)
 
 			err := set(ctx, args[0], args[1])
 			if err != nil {
diff --git a/cmd/publish/main.go b/cmd/publish/main.go
index 5998f16..1639c3a 100644
--- a/cmd/publish/main.go
+++ b/cmd/publish/main.go
@@ -12,7 +12,7 @@ import (
 )
 
 type Messager interface {
-	LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string)
+	LoadVersionMessages(cliVersion string) chan *messager.VersionMessage
 }
 
 type Printer interface {
@@ -53,17 +53,20 @@ func New(ctx *PublishCommandContext) *cobra.Command {
 			}
 			return nil
 		},
+		PreRunE: func(cmd *cobra.Command, args []string) error {
+			outputFlag, _ := cmd.Flags().GetString("output")
+			if (outputFlag != "json") && (outputFlag != "yaml") && (outputFlag != "xml") {
+
+				messages := ctx.Messager.LoadVersionMessages(ctx.CliVersion)
+				for msg := range messages {
+					ctx.Printer.PrintMessage(msg.MessageText+"\n", msg.MessageColor)
+				}
+			}
+			return nil
+		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
 			cmd.SilenceErrors = true
-			messages := make(chan *messager.VersionMessage, 1)
-			go ctx.Messager.LoadVersionMessages(messages, ctx.CliVersion)
-			defer func() {
-				msg, ok := <-messages
-				if ok {
-					ctx.Printer.PrintMessage(msg.MessageText+"\n", msg.MessageColor)
-				}
-			}()
 
 			publishFailedResponse, err := publish(ctx, args[0])
 			if publishFailedResponse != nil {
diff --git a/cmd/version/main.go b/cmd/version/main.go
index f209ce4..6360cec 100644
--- a/cmd/version/main.go
+++ b/cmd/version/main.go
@@ -8,7 +8,7 @@ import (
 )
 
 type Messager interface {
-	LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string)
+	LoadVersionMessages(cliVersion string) chan *messager.VersionMessage
 }
 
 type Printer interface {
@@ -21,8 +21,7 @@ type VersionCommandContext struct {
 }
 
 func version(ctx *VersionCommandContext) {
-	messages := make(chan *messager.VersionMessage, 1)
-	go ctx.Messager.LoadVersionMessages(messages, ctx.CliVersion)
+	messages := ctx.Messager.LoadVersionMessages(ctx.CliVersion)
 	fmt.Println(ctx.CliVersion)
 	msg, ok := <-messages
 	if ok {


Test patch:
diff --git a/bl/messager/messenger_test.go b/bl/messager/messenger_test.go
index b911bc6..11bbbd2 100644
--- a/bl/messager/messenger_test.go
+++ b/bl/messager/messenger_test.go
@@ -50,9 +50,7 @@ func TestLoadVersionMessages(t *testing.T) {
 				defaultTimeout: tt.mock.timeout,
 				messagesClient: mockedMessagesClient,
 			}
-			messager.LoadVersionMessages(tt.args.messagesChan, tt.args.cliVersion)
-
-			msg := <-tt.args.messagesChan
+			msg := <-messager.LoadVersionMessages(tt.args.cliVersion)
 
 			if tt.mock.message != nil {
 				assert.Equal(t, tt.mock.message.CliVersion, msg.CliVersion)
diff --git a/cmd/config/set_test.go b/cmd/config/set_test.go
index 33383da..7b331af 100644
--- a/cmd/config/set_test.go
+++ b/cmd/config/set_test.go
@@ -13,7 +13,8 @@ type mockMessager struct {
 	mock.Mock
 }
 
-func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string) {
+func (m *mockMessager) LoadVersionMessages(cliVersion string) chan *messager.VersionMessage {
+	messages := make(chan *messager.VersionMessage, 1)
 	go func() {
 		messages <- &messager.VersionMessage{
 			CliVersion:   "1.2.3",
@@ -23,6 +24,7 @@ func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessag
 	}()
 
 	m.Called(messages, cliVersion)
+	return messages
 }
 
 func (m *mockMessager) HandleVersionMessage(messageChannel <-chan *messager.VersionMessage) {
@@ -65,7 +67,7 @@ func (lc *LocalConfigMock) Set(key string, value string) error {
 
 func TestSetCommand(t *testing.T) {
 	messager := &mockMessager{}
-	messager.On("LoadVersionMessages", mock.Anything, mock.Anything)
+	messager.On("LoadVersionMessages", mock.Anything)
 
 	printerMock := &PrinterMock{}
 	printerMock.On("PrintWarnings", mock.Anything)
diff --git a/cmd/publish/main_test.go b/cmd/publish/main_test.go
index ed2c24b..a3b77e7 100644
--- a/cmd/publish/main_test.go
+++ b/cmd/publish/main_test.go
@@ -26,7 +26,8 @@ type MessagerMock struct {
 	mock.Mock
 }
 
-func (m *MessagerMock) LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string) {
+func (m *MessagerMock) LoadVersionMessages(cliVersion string) chan *messager.VersionMessage {
+	messages := make(chan *messager.VersionMessage, 1)
 	go func() {
 		messages <- &messager.VersionMessage{
 			CliVersion:   "1.2.3",
@@ -36,6 +37,7 @@ func (m *MessagerMock) LoadVersionMessages(messages chan *messager.VersionMessag
 	}()
 
 	m.Called(messages, cliVersion)
+	return messages
 }
 
 type PrinterMock struct {
@@ -60,7 +62,7 @@ func TestPublishCommand(t *testing.T) {
 	localConfigMock.On("GetLocalConfiguration")
 
 	messagerMock := &MessagerMock{}
-	messagerMock.On("LoadVersionMessages", mock.Anything, mock.Anything)
+	messagerMock.On("LoadVersionMessages", mock.Anything)
 
 	printerMock := &PrinterMock{}
 	printerMock.On("PrintMessage", mock.Anything, mock.Anything)
diff --git a/cmd/test/main.go b/cmd/test/main.go
index 474ef44..0572874 100644
--- a/cmd/test/main.go
+++ b/cmd/test/main.go
@@ -32,7 +32,7 @@ type Evaluator interface {
 }
 
 type Messager interface {
-	LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string)
+	LoadVersionMessages(cliVersion string) chan *messager.VersionMessage
 }
 
 type K8sValidator interface {
@@ -134,6 +134,17 @@ func New(ctx *TestCommandContext) *cobra.Command {
 			}
 			return nil
 		},
+		PreRunE: func(cmd *cobra.Command, args []string) error {
+			outputFlag, _ := cmd.Flags().GetString("output")
+			if (outputFlag != "json") && (outputFlag != "yaml") && (outputFlag != "xml") {
+
+				messages := ctx.Messager.LoadVersionMessages(ctx.CliVersion)
+				for msg := range messages {
+					ctx.Printer.PrintMessage(msg.MessageText+"\n", msg.MessageColor)
+				}
+			}
+			return nil
+		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cmd.SilenceUsage = true
 			cmd.SilenceErrors = true
@@ -327,17 +338,6 @@ func test(ctx *TestCommandContext, paths []string, options *TestCommandOptions)
 func evaluate(ctx *TestCommandContext, filesPaths []string, options *TestCommandOptions) (*ValidationManager, *cliClient.CreateEvaluationResponse, evaluation.ResultType, error) {
 	isInteractiveMode := (options.Output != "json") && (options.Output != "yaml") && (options.Output != "xml")
 
-	if isInteractiveMode {
-		messages := make(chan *messager.VersionMessage, 1)
-		go ctx.Messager.LoadVersionMessages(messages, ctx.CliVersion)
-		defer func() {
-			msg, ok := <-messages
-			if ok {
-				ctx.Printer.PrintMessage(msg.MessageText+"\n", msg.MessageColor)
-			}
-		}()
-	}
-
 	var _spinner *spinner.Spinner
 	if isInteractiveMode && options.Output != "simple" {
 		_spinner = createSpinner(" Loading...", "cyan")
diff --git a/cmd/test/main_test.go b/cmd/test/main_test.go
index 2d36a5d..530b3dc 100644
--- a/cmd/test/main_test.go
+++ b/cmd/test/main_test.go
@@ -43,7 +43,8 @@ type mockMessager struct {
 	mock.Mock
 }
 
-func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string) {
+func (m *mockMessager) LoadVersionMessages(cliVersion string) chan *messager.VersionMessage {
+	messages := make(chan *messager.VersionMessage, 1)
 	go func() {
 		messages <- &messager.VersionMessage{
 			CliVersion:   "1.2.3",
@@ -52,7 +53,8 @@ func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessag
 		close(messages)
 	}()
 
-	m.Called(messages, cliVersion)
+	m.Called(cliVersion)
+	return messages
 }
 
 func (m *mockMessager) HandleVersionMessage(messageChannel <-chan *messager.VersionMessage) {
@@ -141,7 +143,7 @@ func TestTestCommand(t *testing.T) {
 	mockedEvaluator.On("UpdateFailedYamlValidation", mock.Anything, mock.Anything, mock.Anything).Return(nil)
 	mockedEvaluator.On("UpdateFailedK8sValidation", mock.Anything, mock.Anything, mock.Anything).Return(nil)
 	messager := &mockMessager{}
-	messager.On("LoadVersionMessages", mock.Anything, mock.Anything)
+	messager.On("LoadVersionMessages", mock.Anything)
 
 	k8sValidatorMock := &K8sValidatorMock{}
 
diff --git a/cmd/version/main_test.go b/cmd/version/main_test.go
index da4ead1..6d7b62f 100644
--- a/cmd/version/main_test.go
+++ b/cmd/version/main_test.go
@@ -21,7 +21,8 @@ func (p *printerMock) PrintMessage(messageText string, messageColor string) {
 	p.Called(messageText, messageColor)
 }
 
-func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessage, cliVersion string) {
+func (m *mockMessager) LoadVersionMessages(cliVersion string) chan *messager.VersionMessage {
+	messages := make(chan *messager.VersionMessage, 1)
 	go func() {
 		messages <- &messager.VersionMessage{
 			CliVersion:   "0.0.1",
@@ -32,12 +33,13 @@ func (m *mockMessager) LoadVersionMessages(messages chan *messager.VersionMessag
 		close(messages)
 	}()
 
-	m.Called(messages, cliVersion)
+	m.Called(cliVersion)
+	return messages
 }
 
 func Test_ExecuteCommand(t *testing.T) {
 	messager := &mockMessager{}
-	messager.On("LoadVersionMessages", mock.Anything, mock.Anything).Return(nil)
+	messager.On("LoadVersionMessages", mock.Anything).Return(nil)
 
 	printer := &printerMock{}
 	printer.On("PrintMessage", mock.Anything, mock.Anything).Return(nil)