-
Notifications
You must be signed in to change notification settings - Fork 36
fix #201: separate config #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,7 @@ func NewApp(cfg *Config) (app *App, err error) { | |
} | ||
|
||
app.state.SetLogger(app.log) | ||
if err := app.state.Read(cfg.ConfigDir); err != nil { | ||
if err := app.state.Read(cfg.CacheDir); err != nil { | ||
return nil, err | ||
} | ||
|
||
|
@@ -368,11 +368,12 @@ func (app *App) withAppPlayer(ctx context.Context, appPlayerFunc func(context.Co | |
} | ||
|
||
type Config struct { | ||
ConfigDir string `koanf:"config_dir"` | ||
CacheDir string `koanf:"cache"` | ||
ConfigPath string `koanf:"config"` | ||
|
||
// We need to keep this object around, otherwise it gets GC'd and the | ||
// finalizer will run, probably closing the lock. | ||
configLock *flock.Flock | ||
cacheLock *flock.Flock | ||
|
||
LogLevel log.Level `koanf:"log_level"` | ||
LogDisableTimestamp bool `koanf:"log_disable_timestamp"` | ||
|
@@ -424,8 +425,19 @@ type Config struct { | |
} `koanf:"credentials"` | ||
} | ||
|
||
// backwards compatibility for config_dir flag | ||
func aliasNormalizeFunc(f *flag.FlagSet, name string) flag.NormalizedName { | ||
switch name { | ||
case "config_dir": | ||
name = "cache" | ||
break | ||
} | ||
return flag.NormalizedName(name) | ||
} | ||
|
||
func loadConfig(cfg *Config) error { | ||
f := flag.NewFlagSet("config", flag.ContinueOnError) | ||
f.SetNormalizeFunc(aliasNormalizeFunc) | ||
f.Usage = func() { | ||
fmt.Println(f.FlagUsages()) | ||
os.Exit(0) | ||
|
@@ -434,25 +446,33 @@ func loadConfig(cfg *Config) error { | |
if err != nil { | ||
return err | ||
} | ||
defaultConfigDir := filepath.Join(userConfigDir, "go-librespot") | ||
f.StringVar(&cfg.ConfigDir, "config_dir", defaultConfigDir, "the configuration directory") | ||
defaultConfigPath := filepath.Join(userConfigDir, "go-librespot", "config.yaml") | ||
f.StringVar(&cfg.ConfigPath, "config", defaultConfigPath, "the configuration file") | ||
|
||
userCacheDir, err := os.UserCacheDir() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be ideal, yes, but |
||
if err != nil { | ||
return err | ||
} | ||
defaultCachePath := filepath.Join(userCacheDir, "go-librespot") | ||
f.StringVar(&cfg.CacheDir, "cache", defaultCachePath, "the cache directory") | ||
|
||
err = f.Parse(os.Args[1:]) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// Make config directory if needed. | ||
err = os.MkdirAll(cfg.ConfigDir, 0o700) | ||
// Make cache directory if needed. | ||
err = os.MkdirAll(cfg.CacheDir, 0o700) | ||
if err != nil { | ||
return fmt.Errorf("failed creating config directory: %w", err) | ||
return fmt.Errorf("failed creating cache directory: %w", err) | ||
} | ||
|
||
// Lock the config directory (to ensure multiple instances won't clobber | ||
// Lock the cache directory (to ensure multiple instances won't clobber | ||
// each others state). | ||
lockFilePath := filepath.Join(cfg.ConfigDir, "lockfile") | ||
cfg.configLock = flock.New(lockFilePath) | ||
if locked, err := cfg.configLock.TryLock(); err != nil { | ||
return fmt.Errorf("could not lock config directory: %w", err) | ||
lockFilePath := filepath.Join(cfg.CacheDir, "lockfile") | ||
cfg.cacheLock = flock.New(lockFilePath) | ||
if locked, err := cfg.cacheLock.TryLock(); err != nil { | ||
return fmt.Errorf("could not lock cache directory: %w", err) | ||
} else if !locked { | ||
// Lock already taken! Looks like go-librespot is already running. | ||
return fmt.Errorf("%w (lockfile: %s)", errAlreadyRunning, lockFilePath) | ||
|
@@ -481,10 +501,11 @@ func loadConfig(cfg *Config) error { | |
|
||
// load file configuration (if available) | ||
var configPath string | ||
if _, err := os.Stat(filepath.Join(cfg.ConfigDir, "config.yaml")); os.IsNotExist(err) { | ||
configPath = filepath.Join(cfg.ConfigDir, "config.yml") | ||
if _, err := os.Stat(cfg.ConfigPath); os.IsNotExist(err) { | ||
// postel: allow .yml in place of .yaml | ||
configPath = strings.TrimSuffix(cfg.ConfigPath, filepath.Ext(cfg.ConfigPath)) + ".yml" | ||
} else { | ||
configPath = filepath.Join(cfg.ConfigDir, "config.yaml") | ||
configPath = cfg.ConfigPath | ||
} | ||
|
||
if err := k.Load(file.Provider(configPath), yaml.Parser()); err != nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer to explictely keep the
config_dir
option, for backward compatibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I keep it should it follow UserConfigDir or UserCacheDir semantics?
If the former then we have a breaking change for existing deployments of go-librespot.
Either way I take your point - I'll find a way to make it more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making use of
pflag.MarkDeprecated
? (And then unhiding the flag, because pflag autohides it)It doesn't improve things, IMO.
Given go-librespot is pre v1.0 I'd much rather see
config_dir
dropped with prejudice. Not having underscores in flags is an added bonus.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guidcruncher, your view?