Introduce Viper to manage the configuration file #487

masci · 2019-11-18T16:56:49Z

This PR introduces https://github.com/spf13/viper to manage the configuration file in order to:

consistently manage config bindings to env vars and command line switches
get a more generic, key-value API to store/retrieve options
easily get structured config options (see example below)

An example of configuration file would look like this:

board_manager:
  additional_urls:
  - https://raw.githubusercontent.com/ultimachine/ArduinoAddons/master/package_ultimachine_index.json

directories:
  data: /home/massi/.arduino15
  downloads: /home/massi/.arduino15/staging
  libraries: /home/massi/Arduino/libraries
  packages: /home/massi/.arduino15/packages
  sketchbook: /home/massi/Arduino

logging:
  file: ""
  format: text
  level: info

A new package, configuration, has been added to:

keep default values definition in one single module
provide an init mechanism, useful for testing and to allow user customization
wrap common paths definitions

I've left inline comments to ease the review process.

masci · 2019-11-19T10:55:03Z

arduino/cores/packagemanager/loader.go

-		return fmt.Errorf("getting hardware directory: %s", err)
-	}
+
+	dirs = configuration.BundleToolsDirectories()


the error was removed because these functions always returned nil

masci · 2019-11-19T10:57:04Z

cli/cli.go

-	cmd.PersistentFlags().StringVar(&globals.LogLevel, "log-level", defaultLogLevel, "Messages with this level and above will be logged.")
-	cmd.PersistentFlags().StringVar(&logFile, "log-file", "", "Path to the file where logs will be written.")
-	cmd.PersistentFlags().StringVar(&logFormat, "log-format", "text", "The output format for the logs, can be [text|json].")
+	cmd.PersistentFlags().String("log-level", "", "Messages with this level and above will be logged.")


We don't need a var anymore since the flag value is stored in the settings objects directly (this applies to following similar cases)

masci · 2019-11-19T10:57:47Z

cli/cli.go

 	// configure logging filter
-	if lvl, found := toLogLevel(globals.LogLevel); !found {
-		fmt.Printf("Invalid option for --log-level: %s", globals.LogLevel)


unrelated but we should use feedback instead of fmt across the codebase

masci · 2019-11-19T10:58:56Z

cli/config/config.go

- * otherwise use the software for commercial activities involving the Arduino
- * software without disclosing the source code of your own applications. To purchase
- * a commercial license, send an email to license@arduino.cc.
- */


unrelated but we should uniform license format when we find inconsistencies

rsora

I like it, a lot.
We did not lose anything in flexibility and cleaned up a lot of boilerplate, while maintaining retrocompatibility.

configuration/configuration.go

cmaglie · 2019-11-29T11:30:34Z

configs/navigate.go

-)
-
-// Navigate FIXMEDOC
-func (c *Configuration) Navigate(pwd *paths.Path) {


Does viper provides a Navigate feature already?

You can call viper.AddConfigPath() several times passing the path where Viper should look for a config file

cmaglie · 2019-11-29T11:34:04Z

commands/compile/compile.go


 	builderCtx.OtherLibrariesDirs = paths.NewPathList()
-	builderCtx.OtherLibrariesDirs.Add(config.LibrariesDir())
+	builderCtx.OtherLibrariesDirs.Add(paths.New(viper.GetString("directories.Libraries")))


Why not put viper.GetString("directories.Libraries") into an helper function? like as you did with configuration.HardwareDirectories() for example?

At this point I'd write an helper function for each key we put in viper, doing so will enforce a more strict compile-time checks.

One of the reasons to switch to Viper was having a generic k/v store that can arbitrarily grow without the need to change the API. I understand the issue of losing the check on the configuration keys at compile time (if you write viper.GetString("directories.Libs") the program will happily compile despite not working as expected) but I think the tradeoff is worth it. We can catch errors on configuration keys at unit test time.

cmaglie · 2019-11-29T11:38:34Z

configs/preferences_txt_serializer.go

-}
-
-// LoadFromDesktopIDEPreferences loads the config from the Desktop IDE preferences.txt file
-func (config *Configuration) LoadFromDesktopIDEPreferences() error {


Are we dropping loading/overlaying options from the IDE preferences.txt? (just asking I may be OK with that :-))

At first I didn't fully get what this was for - I'd say let's get rid of it.
If needed, I'd add a specific command like arduino-cli config import /path/to/preferences.txt so the operation is explicit.

introduce viper

revert format formatting

rsora

masci added status: in progress Work is in progress on this component/CLI labels Nov 18, 2019

masci force-pushed the massi/viper branch from 5146384 to 58ce3c1 Compare November 19, 2019 10:24

masci commented Nov 19, 2019

View reviewed changes

masci force-pushed the massi/viper branch from 58ce3c1 to c54047d Compare November 19, 2019 15:04

masci removed the status: in progress Work is in progress on this label Nov 19, 2019

masci mentioned this pull request Nov 21, 2019

[proposal][feature-request] Make the daemon port configurable #485

Closed

masci force-pushed the massi/viper branch 3 times, most recently from 6439d3a to 98dbc20 Compare November 28, 2019 10:07

rsora approved these changes Nov 29, 2019

View reviewed changes

cmaglie requested changes Nov 29, 2019

View reviewed changes

masci mentioned this pull request Dec 5, 2019

Compile still unable to compile sketch with version 0.6.0 #503

Closed

Massimiliano Pippi added 13 commits December 10, 2019 17:36

dump all settings

876ccb6

init config file using viper

7dac032

introduce viper

remove configs package

2fe3a6f

add directories module

225de2d

use viper API

0405c55

revert format formatting

bind global option

45abc8f

default url shouldnt be added to additional_urls

66c1d52

map log settings to config file

447fd8e

removed unused global var

6ad2781

fix integration tests

6d51a61

fixed python tests

2dbe9f7

do not use logging before it's ready

764f519

add dep

5e7f2a6

Massimiliano Pippi added 4 commits December 10, 2019 17:38

fixed tests

daf030b

fix IsBundledInDesktopIDE logic

da3cd7f

look for project wide config file

ae52737

update go.mod

2d66c29

masci force-pushed the massi/viper branch from 5a618d5 to 2d66c29 Compare December 10, 2019 16:39

fix integration tests

a863c50

masci force-pushed the massi/viper branch from 7cc8877 to a863c50 Compare December 11, 2019 14:38

Massimiliano Pippi added 3 commits December 11, 2019 16:43

print stdout for test

fe2fce7

print config path

21fa34b

remove debug prints

9cf6c6c

rsora self-requested a review December 11, 2019 16:50

rsora approved these changes Dec 11, 2019

View reviewed changes

masci merged commit f2df284 into master Dec 11, 2019

masci deleted the massi/viper branch December 11, 2019 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Viper to manage the configuration file #487

Introduce Viper to manage the configuration file #487

masci commented Nov 18, 2019 •

edited

masci Nov 19, 2019

masci Nov 19, 2019

masci Nov 19, 2019

masci Nov 19, 2019

rsora left a comment

cmaglie Nov 29, 2019

masci Dec 3, 2019

cmaglie Nov 29, 2019

masci Dec 3, 2019 •

edited

cmaglie Nov 29, 2019

masci Dec 3, 2019

rsora left a comment

Introduce Viper to manage the configuration file #487

Introduce Viper to manage the configuration file #487

Conversation

masci commented Nov 18, 2019 • edited

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

rsora left a 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

masci Dec 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsora left a comment

Choose a reason for hiding this comment

masci commented Nov 18, 2019 •

edited

masci Dec 3, 2019 •

edited