feat: make config directory configurable via flags and env#262
feat: make config directory configurable via flags and env#262kajal-jotwani wants to merge 1 commit intocontainer-registry:mainfrom
Conversation
Signed-off-by: kajal-jotwani <kajaljotwani06@gmail.com>
📝 WalkthroughWalkthroughThis change introduces a configurable config directory via a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/main.go`:
- Around line 70-73: The error print for the os.MkdirAll call (the
fmt.Printf("Error creating config directory: %v", err) next to the configDir
creation) is missing a trailing newline; update that call to include a newline
(e.g., fmt.Printf("Error creating config directory: %v\n", err) or use
fmt.Fprintf(os.Stderr, "...\\n", err)) so it matches the style of the other
error messages in this file.
🧹 Nitpick comments (3)
pkg/config/constants.go (1)
22-28: Silent fallback when home directory is unavailable.When
os.UserHomeDir()fails (e.g., in certain containerized environments or when$HOMEis unset), the function silently falls back to./.config/satellite. This is reasonable behavior, but operators may want visibility into this.Consider whether a log or exported sentinel value would help users diagnose unexpected config locations, or if silent fallback is acceptable for this use case.
cmd/main.go (2)
83-87:InitConfigManageris called twice with identical parameters.The config manager is initialized at line 83 (in
main()) and again at line 115 (inrun()). Both calls use the same arguments and the result from the first call (cm) is only used to extract the zot config for the local registry endpoint.This causes:
- Duplicate file I/O (reading config twice)
- Duplicate validation and processing
- Potential inconsistency if the config file changes between calls
Consider restructuring to initialize once and pass
cmtorun(), or extract only the registry endpoint resolution into a separate lightweight function.Sketch of one approach: pass cm to run()
-func run(jsonLogging bool, token, groundControlURL string, useUnsecure bool, configPath, prevConfigPath string) error { +func run(cm *config.ConfigManager, jsonLogging bool, configPath, prevConfigPath string) error { ctx, cancel := utils.SetupContext(context.Background()) defer cancel() wg, ctx := errgroup.WithContext(ctx) - cm, warnings, err := config.InitConfigManager(token, groundControlURL, configPath, prevConfigPath, jsonLogging, useUnsecure) - if err != nil { - fmt.Printf("Error initiating the config manager: %v", err) - return err - } + warnings := cm.GetInitWarnings() // If warnings need to be surfaced hereThen update the call site in
main():- err = run(jsonLogging, token, groundControlURL, useUnsecure, configPath, prevConfigPath) + err = run(cm, jsonLogging, configPath, prevConfigPath)Also applies to: 115-119
110-110: Function signature has grown—consider a config struct.The
runfunction now accepts 6 parameters. As the parameter count grows, consider grouping related values into a struct (e.g.,RunOptionsor similar) to improve readability and make future additions easier.This is optional and can be deferred if the current signature is acceptable for now.
| if err := os.MkdirAll(configDir, 0o755); err != nil { | ||
| fmt.Printf("Error creating config directory: %v", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Error message missing newline.
The error message on line 71 lacks a trailing newline, which is inconsistent with similar error messages in this file (e.g., lines 85, 99, 105).
Proposed fix
if err := os.MkdirAll(configDir, 0o755); err != nil {
- fmt.Printf("Error creating config directory: %v", err)
+ fmt.Printf("Error creating config directory: %v\n", err)
os.Exit(1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := os.MkdirAll(configDir, 0o755); err != nil { | |
| fmt.Printf("Error creating config directory: %v", err) | |
| os.Exit(1) | |
| } | |
| if err := os.MkdirAll(configDir, 0o755); err != nil { | |
| fmt.Printf("Error creating config directory: %v\n", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/main.go` around lines 70 - 73, The error print for the os.MkdirAll call
(the fmt.Printf("Error creating config directory: %v", err) next to the
configDir creation) is missing a trailing newline; update that call to include a
newline (e.g., fmt.Printf("Error creating config directory: %v\n", err) or use
fmt.Fprintf(os.Stderr, "...\\n", err)) so it matches the style of the other
error messages in this file.
1 participant
Description
This PR allows users to configure where Satellite stores its config and state files using a
--config-dirflag or theCONFIG_DIRenvironment variable.If neither is provided, Satellite defaults to
~/.config/satelliteand ensures the directory exists before use.Related Issue: #226
Summary by cubic
Make the Satellite config directory configurable via a new --config-dir flag and CONFIG_DIR env. Defaults to ~/.config/satellite, ensures the directory exists, and uses it for config reads and file watching.
New Features
Migration
Written for commit 4c16cef. Summary will update on new commits.
Summary by CodeRabbit
--config-dircommand-line flag to specify custom configuration directory location.CONFIG_DIRenvironment variable support for configuration directory specification.✏️ Tip: You can customize this high-level summary in your review settings.