Implement configuration system #231
Conversation
frrist
requested review from
Peeja,
alanshaw,
hannahhoward and
volmedo
as code owners
Peeja
left a comment
Peeja
left a comment
There was a problem hiding this comment.
This mostly looks great! A couple of notes worth addressing, and some conflicts to resolve, but I'll leave it all to your discretion.
| return fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
||
| c := cmdutil.MustGetClient(storePath, client.WithAdditionalProofs(proofs...)) |
There was a problem hiding this comment.
question: Did we mean to lose the additional proofs here?
| setSpanAttributes(cmd, span) | ||
|
|
||
| storePath = filepath.Join(guppyDirPath, "store.json") | ||
| cfg, err := config.Load() |
There was a problem hiding this comment.
question: If we're doing this here already, can we "pass down" the config somehow, or do we have to config.Load() in each command that needs it as well?
|
|
||
| func initFlags() { | ||
| rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", | ||
| "Config file path. Attempts to load from user config directory if not set e.g. ~/.config/"+defaultConfigFilePath) |
|
|
||
| func init() { | ||
| cobra.EnableTraverseRunHooks = true | ||
| // Register flags/commands before Cobra parses args so subcommands are available. |
There was a problem hiding this comment.
question: Can you say more? I'm not following.
| viper.SetEnvPrefix("GUPPY") | ||
| viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) | ||
|
|
||
| // if no flag was provided look in $HOME/.config/guppy/config.toml |
There was a problem hiding this comment.
suggestion: This isn't accurate on macOS (or some other OSes).
| // if no flag was provided look in $HOME/.config/guppy/config.toml | |
| // if no flag was provided look in <os.UserConfigDir()>/guppy/config.toml |
That said, using ~/Library/Application Support/guppy feels weird, too.
| if err != nil { | ||
| return err | ||
| } | ||
| // FIXME: We should fix this! |
There was a problem hiding this comment.
Yeah, I think this is about to be solved with the UI overhaul.
| } | ||
|
|
||
| func (r RepoConfig) PreparationDatabaseFilePath() string { | ||
| return filepath.Join(r.DataDir, "preparation.db") |
There was a problem hiding this comment.
question: Is this no longer independently configurable? I'm not against that, just confirming. It was probably too flexible before.
| Identity IdentityConfig `mapstruct:"identity"` | ||
| Repo RepoConfig `mapstruct:"repo"` | ||
| UploadService ServiceConfig `mapstruct:"upload_service"` | ||
| IndexingService ServiceConfig `mapstruct:"indexing_service"` |
There was a problem hiding this comment.
question/suggestion: Should this be
| Identity IdentityConfig `mapstruct:"identity"` | |
| Repo RepoConfig `mapstruct:"repo"` | |
| UploadService ServiceConfig `mapstruct:"upload_service"` | |
| IndexingService ServiceConfig `mapstruct:"indexing_service"` | |
| Identity IdentityConfig `mapstructure:"identity"` | |
| Repo RepoConfig `mapstructure:"repo"` | |
| UploadService ServiceConfig `mapstructure:"upload_service"` | |
| IndexingService ServiceConfig `mapstructure:"indexing_service"` |
?
| func (r RepoConfig) AgentDataFilePath() string { | ||
| return filepath.Join(r.DataDir, "store.json") | ||
| } |
There was a problem hiding this comment.
issue: This seems off to me. You can have agent data when there's no repo. The repo/db is only used for guppy upload. Lots of commands use the agent data.
No description provided.