From eb82eb8c1d4fae625833187ba37ccf1ff88a2fd5 Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Fri, 18 Sep 2020 11:51:42 +0200 Subject: [PATCH 1/3] Change behaviour of --config-file flag with config commands --- cli/config/dump.go | 17 +++++++ cli/config/init.go | 52 ++++++++++++++++---- test/test_config.py | 115 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 171 insertions(+), 13 deletions(-) diff --git a/cli/config/dump.go b/cli/config/dump.go index c384a8d48a7..1a93f871181 100644 --- a/cli/config/dump.go +++ b/cli/config/dump.go @@ -18,6 +18,7 @@ package config import ( "os" + "github.com/arduino/arduino-cli/cli/errorcodes" "github.com/arduino/arduino-cli/cli/feedback" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -59,5 +60,21 @@ func (dr dumpResult) String() string { func runDumpCommand(cmd *cobra.Command, args []string) { logrus.Info("Executing `arduino config dump`") + + configFlag := cmd.InheritedFlags().Lookup("config-file") + configFile := "" + if configFlag != nil { + configFile = configFlag.Value.String() + } + + if configFile != "" { + viper.SetConfigFile(configFile) + err := viper.MergeInConfig() + if err != nil { + feedback.Errorf("Error reading config file: %s", configFile) + os.Exit(errorcodes.ErrBadArgument) + } + } + feedback.PrintResult(dumpResult{viper.AllSettings()}) } diff --git a/cli/config/init.go b/cli/config/init.go index 8d0cbc66559..bf49236f99b 100644 --- a/cli/config/init.go +++ b/cli/config/init.go @@ -17,16 +17,17 @@ package config import ( "os" - "path/filepath" "github.com/arduino/arduino-cli/cli/errorcodes" "github.com/arduino/arduino-cli/cli/feedback" + paths "github.com/arduino/go-paths-helper" "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/spf13/viper" ) var destDir string +var overwrite bool const defaultFileName = "arduino-cli.yaml" @@ -42,34 +43,65 @@ func initInitCommand() *cobra.Command { Run: runInitCommand, } initCommand.Flags().StringVar(&destDir, "dest-dir", "", "Sets where to save the configuration file.") + initCommand.Flags().BoolVar(&overwrite, "overwrite", false, "Overwrite existing config file.") return initCommand } func runInitCommand(cmd *cobra.Command, args []string) { - if destDir == "" { + configFlag := cmd.InheritedFlags().Lookup("config-file") + configFilePath := "" + + if configFlag != nil { + configFilePath = configFlag.Value.String() + } + + if configFilePath != "" && destDir != "" { + feedback.Errorf("Can't use both --config-file and --dest-dir flags at the same time.") + os.Exit(errorcodes.ErrGeneric) + } + + var configFileAbsPath *paths.Path + var absPath *paths.Path + var err error + + switch { + case configFilePath != "": + configFileAbsPath, err = paths.New(configFilePath).Abs() + if err != nil { + feedback.Errorf("Cannot find absolute path: %v", err) + os.Exit(errorcodes.ErrGeneric) + } + + absPath = configFileAbsPath.Parent() + case destDir == "": destDir = viper.GetString("directories.Data") + fallthrough + default: + absPath, err = paths.New(destDir).Abs() + if err != nil { + feedback.Errorf("Cannot find absolute path: %v", err) + os.Exit(errorcodes.ErrGeneric) + } + configFileAbsPath = absPath.Join(defaultFileName) } - absPath, err := filepath.Abs(destDir) - if err != nil { - feedback.Errorf("Cannot find absolute path: %v", err) + if !overwrite && configFileAbsPath.Exist() { + feedback.Error("Config file already exists, use --overwrite to discard the existing one.") os.Exit(errorcodes.ErrGeneric) } - configFileAbsPath := filepath.Join(absPath, defaultFileName) logrus.Infof("Writing config file to: %s", absPath) - - if err := os.MkdirAll(absPath, os.FileMode(0755)); err != nil { + if err := absPath.MkdirAll(); err != nil { feedback.Errorf("Cannot create config file directory: %v", err) os.Exit(errorcodes.ErrGeneric) } - if err := viper.WriteConfigAs(configFileAbsPath); err != nil { + if err := viper.WriteConfigAs(configFileAbsPath.String()); err != nil { feedback.Errorf("Cannot create config file: %v", err) os.Exit(errorcodes.ErrGeneric) } - msg := "Config file written to: " + configFileAbsPath + msg := "Config file written to: " + configFileAbsPath.String() logrus.Info(msg) feedback.Print(msg) } diff --git a/test/test_config.py b/test/test_config.py index 28320b3d40f..dc01de12ae0 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -13,6 +13,7 @@ # software without disclosing the source code of your own applications. To purchase # a commercial license, send an email to license@arduino.cc. from pathlib import Path +import json def test_init(run_command, data_dir, working_dir): @@ -21,8 +22,116 @@ def test_init(run_command, data_dir, working_dir): assert data_dir in result.stdout -def test_init_dest(run_command, working_dir): - dest = str(Path(working_dir) / "config" / "test") +def test_init_dest_absolute_path(run_command, working_dir): + dest = Path(working_dir) / "config" / "test" + expected_config_file = dest / "arduino-cli.yaml" + assert not expected_config_file.exists() result = run_command(f'config init --dest-dir "{dest}"') assert result.ok - assert dest in result.stdout + assert str(expected_config_file) in result.stdout + assert expected_config_file.exists() + + +def test_init_dest_relative_path(run_command, working_dir): + dest = Path(working_dir) / "config" / "test" + expected_config_file = dest / "arduino-cli.yaml" + assert not expected_config_file.exists() + result = run_command('config init --dest-dir "config/test"') + assert result.ok + assert str(expected_config_file) in result.stdout + assert expected_config_file.exists() + + +def test_init_dest_flag_with_overwrite_flag(run_command, working_dir): + dest = Path(working_dir) / "config" / "test" + + expected_config_file = dest / "arduino-cli.yaml" + assert not expected_config_file.exists() + + result = run_command(f'config init --dest-dir "{dest}"') + assert result.ok + assert expected_config_file.exists() + + result = run_command(f'config init --dest-dir "{dest}"') + assert result.failed + assert "Config file already exists, use --overwrite to discard the existing one." in result.stderr + + result = run_command(f'config init --dest-dir "{dest}" --overwrite') + assert result.ok + assert str(expected_config_file) in result.stdout + + +def test_init_dest_and_config_file_flags(run_command, working_dir): + result = run_command('config init --config-file "some_other_path" --dest-dir "some_path"') + assert result.failed + assert "Can't use both --config-file and --dest-dir flags at the same time." in result.stderr + + +def test_init_config_file_flag_absolute_path(run_command, working_dir): + config_file = Path(working_dir) / "config" / "test" / "config.yaml" + assert not config_file.exists() + result = run_command(f'config init --config-file "{config_file}"') + assert result.ok + assert str(config_file) in result.stdout + assert config_file.exists() + + +def test_init_config_file_flag_relative_path(run_command, working_dir): + config_file = Path(working_dir) / "config.yaml" + assert not config_file.exists() + result = run_command('config init --config-file "config.yaml"') + assert result.ok + assert str(config_file) in result.stdout + assert config_file.exists() + + +def test_init_config_file_flag_with_overwrite_flag(run_command, working_dir): + config_file = Path(working_dir) / "config" / "test" / "config.yaml" + assert not config_file.exists() + + result = run_command(f'config init --config-file "{config_file}"') + assert result.ok + assert config_file.exists() + + result = run_command(f'config init --config-file "{config_file}"') + assert result.failed + assert "Config file already exists, use --overwrite to discard the existing one." in result.stderr + + result = run_command(f'config init --config-file "{config_file}" --overwrite') + assert result.ok + assert str(config_file) in result.stdout + + +def test_dump(run_command, working_dir): + # Create a config file first + config_file = Path(working_dir) / "config" / "test" / "config.yaml" + assert not config_file.exists() + result = run_command(f'config init --config-file "{config_file}"') + assert result.ok + assert config_file.exists() + + result = run_command("config dump --format json") + assert result.ok + settings_json = json.loads(result.stdout) + assert [] == settings_json["board_manager"]["additional_urls"] + + +def test_dump_with_config_file_flag(run_command, working_dir): + # Create a config file first + config_file = Path(working_dir) / "config" / "test" / "config.yaml" + assert not config_file.exists() + result = run_command(f'config init --config-file "{config_file}" --additional-urls=https://example.com') + assert result.ok + assert config_file.exists() + + result = run_command(f'config dump --config-file "{config_file}" --format json') + assert result.ok + settings_json = json.loads(result.stdout) + assert ["https://example.com"] == settings_json["board_manager"]["additional_urls"] + + result = run_command( + f'config dump --config-file "{config_file}" --additional-urls=https://another-url.com --format json' + ) + assert result.ok + settings_json = json.loads(result.stdout) + assert ["https://another-url.com"] == settings_json["board_manager"]["additional_urls"] From 90300e970765ff895ea314df61d7fa17a4e35111 Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Mon, 21 Sep 2020 14:37:04 +0200 Subject: [PATCH 2/3] Fix issue when initializing new config file if one already exists --- configuration/configuration.go | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/configuration/configuration.go b/configuration/configuration.go index 02924d3de81..b45a42601d4 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -198,15 +198,10 @@ func FindConfigFile() string { } if configFile != "" { - if fi, err := os.Stat(configFile); err == nil { - if fi.IsDir() { - return configFile - } - return filepath.Dir(configFile) - } + return filepath.Dir(configFile) } - return searchCwdForConfig() + return "" } func searchConfigTree(cwd string) string { @@ -230,13 +225,3 @@ func searchConfigTree(cwd string) string { } } - -func searchCwdForConfig() string { - cwd, err := os.Getwd() - - if err != nil { - return "" - } - - return searchConfigTree(cwd) -} From e0bc70a9c1c525c6157eda24ebf1f581267d81c6 Mon Sep 17 00:00:00 2001 From: Silvano Cerza Date: Thu, 1 Oct 2020 12:52:51 +0200 Subject: [PATCH 3/3] Change how config file path is initialized --- cli/config/dump.go | 17 ------------ cli/config/init.go | 27 +++++++++--------- commands/daemon/settings_test.go | 5 ++-- configuration/configuration.go | 38 +++++++++++++++++++------ configuration/configuration_test.go | 43 +++++++++++++++++++++++++++++ main.go | 2 +- test/test_config.py | 18 ++++++------ 7 files changed, 98 insertions(+), 52 deletions(-) diff --git a/cli/config/dump.go b/cli/config/dump.go index 1a93f871181..c384a8d48a7 100644 --- a/cli/config/dump.go +++ b/cli/config/dump.go @@ -18,7 +18,6 @@ package config import ( "os" - "github.com/arduino/arduino-cli/cli/errorcodes" "github.com/arduino/arduino-cli/cli/feedback" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -60,21 +59,5 @@ func (dr dumpResult) String() string { func runDumpCommand(cmd *cobra.Command, args []string) { logrus.Info("Executing `arduino config dump`") - - configFlag := cmd.InheritedFlags().Lookup("config-file") - configFile := "" - if configFlag != nil { - configFile = configFlag.Value.String() - } - - if configFile != "" { - viper.SetConfigFile(configFile) - err := viper.MergeInConfig() - if err != nil { - feedback.Errorf("Error reading config file: %s", configFile) - os.Exit(errorcodes.ErrBadArgument) - } - } - feedback.PrintResult(dumpResult{viper.AllSettings()}) } diff --git a/cli/config/init.go b/cli/config/init.go index bf49236f99b..494263f2b82 100644 --- a/cli/config/init.go +++ b/cli/config/init.go @@ -26,8 +26,11 @@ import ( "github.com/spf13/viper" ) -var destDir string -var overwrite bool +var ( + destDir string + destFile string + overwrite bool +) const defaultFileName = "arduino-cli.yaml" @@ -38,25 +41,21 @@ func initInitCommand() *cobra.Command { Long: "Creates or updates the configuration file in the data directory or custom directory with the current configuration settings.", Example: "" + " # Writes current configuration to the configuration file in the data directory.\n" + - " " + os.Args[0] + " config init", + " " + os.Args[0] + " config init" + + " " + os.Args[0] + " config init --dest-dir /home/user/MyDirectory" + + " " + os.Args[0] + " config init --dest-file /home/user/MyDirectory/my_settings.yaml", Args: cobra.NoArgs, Run: runInitCommand, } initCommand.Flags().StringVar(&destDir, "dest-dir", "", "Sets where to save the configuration file.") + initCommand.Flags().StringVar(&destFile, "dest-file", "", "Sets where to save the configuration file.") initCommand.Flags().BoolVar(&overwrite, "overwrite", false, "Overwrite existing config file.") return initCommand } func runInitCommand(cmd *cobra.Command, args []string) { - configFlag := cmd.InheritedFlags().Lookup("config-file") - configFilePath := "" - - if configFlag != nil { - configFilePath = configFlag.Value.String() - } - - if configFilePath != "" && destDir != "" { - feedback.Errorf("Can't use both --config-file and --dest-dir flags at the same time.") + if destFile != "" && destDir != "" { + feedback.Errorf("Can't use both --dest-file and --dest-dir flags at the same time.") os.Exit(errorcodes.ErrGeneric) } @@ -65,8 +64,8 @@ func runInitCommand(cmd *cobra.Command, args []string) { var err error switch { - case configFilePath != "": - configFileAbsPath, err = paths.New(configFilePath).Abs() + case destFile != "": + configFileAbsPath, err = paths.New(destFile).Abs() if err != nil { feedback.Errorf("Cannot find absolute path: %v", err) os.Exit(errorcodes.ErrGeneric) diff --git a/commands/daemon/settings_test.go b/commands/daemon/settings_test.go index 4bc7a11f86f..512b8d7a779 100644 --- a/commands/daemon/settings_test.go +++ b/commands/daemon/settings_test.go @@ -18,6 +18,7 @@ package daemon import ( "context" "encoding/json" + "path/filepath" "testing" "github.com/spf13/viper" @@ -30,12 +31,12 @@ import ( var svc = SettingsService{} func init() { - configuration.Init("testdata") + configuration.Init(filepath.Join("testdata", "arduino-cli.yaml")) } func reset() { viper.Reset() - configuration.Init("testdata") + configuration.Init(filepath.Join("testdata", "arduino-cli.yaml")) } func TestGetAll(t *testing.T) { diff --git a/configuration/configuration.go b/configuration/configuration.go index b45a42601d4..d011d63fbed 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -35,7 +35,13 @@ import ( func Init(configPath string) { // Config file metadata jww.SetStdoutThreshold(jww.LevelFatal) - viper.SetConfigName("arduino-cli") + + configDir := paths.New(configPath) + if configDir != nil && !configDir.IsDir() { + viper.SetConfigName(strings.TrimSuffix(configDir.Base(), configDir.Ext())) + } else { + viper.SetConfigName("arduino-cli") + } // Get default data path if none was provided if configPath == "" { @@ -43,7 +49,7 @@ func Init(configPath string) { } // Add paths where to search for a config file - viper.AddConfigPath(configPath) + viper.AddConfigPath(filepath.Dir(configPath)) // Bind env vars viper.SetEnvPrefix("ARDUINO") @@ -185,23 +191,37 @@ func IsBundledInDesktopIDE() bool { } // FindConfigFile returns the config file path using the argument '--config-file' if specified or via the current working dir -func FindConfigFile() string { - +func FindConfigFile(args []string) string { configFile := "" - for i, arg := range os.Args { + for i, arg := range args { // 0 --config-file ss if arg == "--config-file" { - if len(os.Args) > i+1 { - configFile = os.Args[i+1] + if len(args) > i+1 { + configFile = args[i+1] } } } if configFile != "" { - return filepath.Dir(configFile) + return configFile + } + + return searchCwdForConfig() +} + +func searchCwdForConfig() string { + cwd, err := os.Getwd() + + if err != nil { + return "" + } + + configFile := searchConfigTree(cwd) + if configFile == "" { + return configFile } - return "" + return configFile + string(os.PathSeparator) + "arduino-cli.yaml" } func searchConfigTree(cwd string) string { diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go index 820d19c2f50..768038b1a62 100644 --- a/configuration/configuration_test.go +++ b/configuration/configuration_test.go @@ -30,6 +30,12 @@ func tmpDirOrDie() string { if err != nil { panic(fmt.Sprintf("error creating tmp dir: %v", err)) } + // Symlinks are evaluated becase the temp folder on Mac OS is inside /var, it's not writable + // and is a symlink to /private/var, we want the full path so we do this + dir, err = filepath.EvalSymlinks(dir) + if err != nil { + panic(fmt.Sprintf("error evaluating tmp dir symlink: %v", err)) + } return dir } @@ -71,3 +77,40 @@ func BenchmarkSearchConfigTree(b *testing.B) { } result = s } + +func TestFindConfigFile(t *testing.T) { + configFile := FindConfigFile([]string{"--config-file"}) + require.Equal(t, "", configFile) + + configFile = FindConfigFile([]string{"--config-file", "some/path/to/config"}) + require.Equal(t, "some/path/to/config", configFile) + + configFile = FindConfigFile([]string{"--config-file", "some/path/to/config/arduino-cli.yaml"}) + require.Equal(t, "some/path/to/config/arduino-cli.yaml", configFile) + + configFile = FindConfigFile([]string{}) + require.Equal(t, "", configFile) + + // Create temporary directories + tmp := tmpDirOrDie() + defer os.RemoveAll(tmp) + target := filepath.Join(tmp, "foo", "bar", "baz") + os.MkdirAll(target, os.ModePerm) + require.Nil(t, os.Chdir(target)) + + // Create a config file + f, err := os.Create(filepath.Join(target, "..", "..", "arduino-cli.yaml")) + require.Nil(t, err) + f.Close() + + configFile = FindConfigFile([]string{}) + require.Equal(t, filepath.Join(tmp, "foo", "arduino-cli.yaml"), configFile) + + // Create another config file + f, err = os.Create(filepath.Join(target, "arduino-cli.yaml")) + require.Nil(t, err) + f.Close() + + configFile = FindConfigFile([]string{}) + require.Equal(t, filepath.Join(target, "arduino-cli.yaml"), configFile) +} diff --git a/main.go b/main.go index d4a43cf81c8..6e45108777f 100644 --- a/main.go +++ b/main.go @@ -25,7 +25,7 @@ import ( ) func main() { - configuration.Init(configuration.FindConfigFile()) + configuration.Init(configuration.FindConfigFile(os.Args)) i18n.Init() arduinoCmd := cli.NewCommand() if err := arduinoCmd.Execute(); err != nil { diff --git a/test/test_config.py b/test/test_config.py index dc01de12ae0..335fceda789 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -62,15 +62,15 @@ def test_init_dest_flag_with_overwrite_flag(run_command, working_dir): def test_init_dest_and_config_file_flags(run_command, working_dir): - result = run_command('config init --config-file "some_other_path" --dest-dir "some_path"') + result = run_command('config init --dest-file "some_other_path" --dest-dir "some_path"') assert result.failed - assert "Can't use both --config-file and --dest-dir flags at the same time." in result.stderr + assert "Can't use both --dest-file and --dest-dir flags at the same time." in result.stderr def test_init_config_file_flag_absolute_path(run_command, working_dir): config_file = Path(working_dir) / "config" / "test" / "config.yaml" assert not config_file.exists() - result = run_command(f'config init --config-file "{config_file}"') + result = run_command(f'config init --dest-file "{config_file}"') assert result.ok assert str(config_file) in result.stdout assert config_file.exists() @@ -79,7 +79,7 @@ def test_init_config_file_flag_absolute_path(run_command, working_dir): def test_init_config_file_flag_relative_path(run_command, working_dir): config_file = Path(working_dir) / "config.yaml" assert not config_file.exists() - result = run_command('config init --config-file "config.yaml"') + result = run_command('config init --dest-file "config.yaml"') assert result.ok assert str(config_file) in result.stdout assert config_file.exists() @@ -89,15 +89,15 @@ def test_init_config_file_flag_with_overwrite_flag(run_command, working_dir): config_file = Path(working_dir) / "config" / "test" / "config.yaml" assert not config_file.exists() - result = run_command(f'config init --config-file "{config_file}"') + result = run_command(f'config init --dest-file "{config_file}"') assert result.ok assert config_file.exists() - result = run_command(f'config init --config-file "{config_file}"') + result = run_command(f'config init --dest-file "{config_file}"') assert result.failed assert "Config file already exists, use --overwrite to discard the existing one." in result.stderr - result = run_command(f'config init --config-file "{config_file}" --overwrite') + result = run_command(f'config init --dest-file "{config_file}" --overwrite') assert result.ok assert str(config_file) in result.stdout @@ -106,7 +106,7 @@ def test_dump(run_command, working_dir): # Create a config file first config_file = Path(working_dir) / "config" / "test" / "config.yaml" assert not config_file.exists() - result = run_command(f'config init --config-file "{config_file}"') + result = run_command(f'config init --dest-file "{config_file}"') assert result.ok assert config_file.exists() @@ -120,7 +120,7 @@ def test_dump_with_config_file_flag(run_command, working_dir): # Create a config file first config_file = Path(working_dir) / "config" / "test" / "config.yaml" assert not config_file.exists() - result = run_command(f'config init --config-file "{config_file}" --additional-urls=https://example.com') + result = run_command(f'config init --dest-file "{config_file}" --additional-urls=https://example.com') assert result.ok assert config_file.exists()