Skip to content
This repository was archived by the owner on Dec 21, 2023. It is now read-only.

fix(cli): switch automatic version check based on config #5290

Merged
bacherfl merged 1 commit into from
Oct 15, 2021
Merged

fix(cli): switch automatic version check based on config #5290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 18 additions & 20 deletions cli/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ import (
"os"
"strings"

"github.com/keptn/keptn/cli/pkg/config"
"github.com/keptn/keptn/cli/pkg/logging"
"github.com/keptn/keptn/cli/pkg/version"
homedir "github.com/mitchellh/go-homedir"
"github.com/spf13/cobra"
"github.com/spf13/viper"
)

var cfgFile string
Expand All @@ -26,6 +25,7 @@ const authErrorMsg = "This command requires to be authenticated. See \"keptn aut

// rootCmd represents the base command when called without any subcommands
var rootCmd = NewRootCommand(version.NewVersionChecker())
var rootCLIConfig config.CLIConfig

func NewRootCommand(vChecker *version.VersionChecker) *cobra.Command {
rootCmd := &cobra.Command{
Expand All @@ -34,7 +34,7 @@ func NewRootCommand(vChecker *version.VersionChecker) *cobra.Command {
Long: `The CLI allows interaction with a Keptn installation to manage Keptn, to trigger workflows, and to get details.
`,
PersistentPreRun: func(cmd *cobra.Command, args []string) {
runVersionCheck(vChecker, os.Args[1:])
runVersionCheck(vChecker, os.Args[1:], rootCLIConfig)
Copy link
Copy Markdown
Member Author

@vadasambar vadasambar Sep 30, 2021

Choose a reason for hiding this comment

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

Passing rootCLIConfig just makes it easier to test runVersionCheck

},
}
return rootCmd
Expand Down Expand Up @@ -81,27 +81,19 @@ func initConfig() {
logging.LogLevel = logging.QuietLevel
}

cfgMgr := config.NewCLIConfigManager()
Copy link
Copy Markdown
Member Author

@vadasambar vadasambar Sep 30, 2021

Choose a reason for hiding this comment

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

I have used CLIConfigManager here instead of viper because we already have it in place and it works well.
TODO for the future can be to re-write NewCLIConfigManager with viper.


if cfgFile != "" {
// Use config file from the flag.
viper.SetConfigFile(cfgFile)
} else {
// Find home directory.
home, err := homedir.Dir()
if err != nil {
fmt.Println(err)
os.Exit(1)
}

// Search config in home directory with name ".cli" (without extension).
viper.AddConfigPath(home)
viper.SetConfigName(".cli")
cfgMgr.CLIConfigPath = cfgFile
}

viper.AutomaticEnv() // read in environment variables that match
logging.PrintLog(fmt.Sprintf("Using config file: %s", cfgMgr.CLIConfigPath), logging.InfoLevel)
Copy link
Copy Markdown
Member

@bacherfl bacherfl Oct 14, 2021

Choose a reason for hiding this comment

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

I think we can remove this log output. Other than that the PR looks good :)

Copy link
Copy Markdown
Member Author

@vadasambar vadasambar Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Printing the above log message is the default behavior. Do you want me to change this? If yes, I would like to know the reason. Thank you!

Copy link
Copy Markdown
Member

@bacherfl bacherfl Oct 15, 2021

Choose a reason for hiding this comment

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

The main reason I was thinking of is that it might interfere with pipelines where the keptn CLI is used within scripts - but I just checked that the --quiet flag is applied correctly here, meaning that the message is not shown if this flag is set. So therefore we can leave it in there


// If a config file is found, read it in.
if err := viper.ReadInConfig(); err == nil {
logging.PrintLog(fmt.Sprintf("Using config file: %s", viper.ConfigFileUsed()), logging.InfoLevel)
var err error
rootCLIConfig, err = cfgMgr.LoadCLIConfig()
if err != nil {
logging.PrintLog(err.Error(), logging.InfoLevel)
}
}

Expand All @@ -113,7 +105,13 @@ func (s *options) appendIfNotEmpty(newOption string) {
}
}

func runVersionCheck(vChecker *version.VersionChecker, flags []string) {
// passing flags and cliConfig as arguments makes it easy to test this function
func runVersionCheck(vChecker *version.VersionChecker, flags []string, cliConfig config.CLIConfig) {
// Don't check version if AutomaticVersionCheck is disabled
if !cliConfig.AutomaticVersionCheck {
return
}

// Server version won't be available during `install`
// because the Server is not installed yet
if isInstallSubCommand(flags) {
Expand Down
42 changes: 40 additions & 2 deletions cli/cmd/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"testing"

keptnapimodels "github.com/keptn/go-utils/pkg/api/models"
"github.com/keptn/keptn/cli/pkg/config"
"github.com/keptn/keptn/cli/pkg/version"
shellwords "github.com/mattn/go-shellwords"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -71,7 +72,7 @@ func executeActionCommandC(cmd string) (string, error) {
},
}
rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) {
runVersionCheck(vChecker, os.Args[1:])
runVersionCheck(vChecker, os.Args[1:], rootCLIConfig)
}

rootCmd.SetOut(buf)
Expand Down Expand Up @@ -183,6 +184,7 @@ func Test_runVersionCheck(t *testing.T) {
args []string
wantOutput string
doNotWantOutput string
cliConfig func() config.CLIConfig
}{
{
name: "get version",
Expand Down Expand Up @@ -242,6 +244,34 @@ func Test_runVersionCheck(t *testing.T) {
},
doNotWantOutput: "* Warning: Your Keptn CLI version (0.8.0) and Keptn cluster version (0.8.0) don't match.",
},
{
name: "show version check if `AutomaticVersionCheck` is set to true",
cliVersion: "0.8.0",
metadataStatus: http.StatusOK,
metadataResponse: keptnapimodels.Metadata{
Keptnversion: "0.8.1-dev",
},
wantOutput: "* Warning: Your Keptn CLI version (0.8.0) and Keptn cluster version (0.8.1-dev) don't match.",
cliConfig: func() config.CLIConfig {
Copy link
Copy Markdown
Member Author

@vadasambar vadasambar Sep 30, 2021

Choose a reason for hiding this comment

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

Doing this because changing rootCLIConfig directly can have impact on other tests.

cliConfig := rootCLIConfig
cliConfig.AutomaticVersionCheck = true
return cliConfig
},
},
{
name: "don't show version check if `AutomaticVersionCheck` is set to false",
cliVersion: "0.8.0",
metadataStatus: http.StatusOK,
metadataResponse: keptnapimodels.Metadata{
Keptnversion: "0.8.1-dev",
},
cliConfig: func() config.CLIConfig {
cliConfig := rootCLIConfig
cliConfig.AutomaticVersionCheck = false
return cliConfig
},
doNotWantOutput: "* Warning: Your Keptn CLI version (0.8.0) and Keptn cluster version (0.8.1-dev) don't match.",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -262,7 +292,15 @@ func Test_runVersionCheck(t *testing.T) {
VersionURL: ts.URL,
},
}
runVersionCheck(vChecker, tt.args)

var cliConfig config.CLIConfig
if tt.cliConfig == nil {
cliConfig = rootCLIConfig
} else {
cliConfig = tt.cliConfig()
}

runVersionCheck(vChecker, tt.args, cliConfig)

// reset version
Version = ""
Expand Down