Skip to content

feat: add config.yaml support for timeouts, concurrency, persistence#79

Merged
yimsk merged 12 commits intodevelopfrom
feat/issue-67
Jan 2, 2026
Merged

feat: add config.yaml support for timeouts, concurrency, persistence#79
yimsk merged 12 commits intodevelopfrom
feat/issue-67

Conversation

@yimsk
Copy link
Copy Markdown
Contributor

@yimsk yimsk commented Jan 2, 2026

Summary

  • Add ~/.config/claws/config.yaml support for customizable timeouts, concurrency limits, CloudWatch window, and startup profile/region persistence
  • Unify hardcoded constants (awsInitTimeout, maxConcurrentProfileFetches) into configurable FileConfig with thread-safe accessors
  • Add --persist/--no-persist CLI flags to override config file persistence setting

Changes

New Features

  • Config file support (internal/config/file.go): YAML-based config with Duration type for human-readable timeouts
  • Configurable timeouts: aws_init, multi_region_fetch, tag_search, metrics_load
  • Concurrency limit: max_fetches (default: 50)
  • CloudWatch window: cloudwatch.window (default: 15m)
  • Persistence: Save region/profile selection on change when enabled
  • Startup config: Auto-apply profile/regions on launch

Refactoring

  • Extract lock helpers (withRLock, doWithLock) to internal/config/lock.go
  • Remove duplicate constants from internal/app/app.go and internal/aws/init.go
  • Atomic config save via temp file + rename pattern

Documentation

  • README updated with config file example and CLI flags

Testing

  • 284 lines of new tests in internal/config/file_test.go
  • Duration marshal/unmarshal, save/load roundtrip, partial config, negative value handling
  • All existing tests pass

Closes #67, closes #50

yimsk added 10 commits January 2, 2026 11:48
…67, #50)

- Add ~/.config/claws/config.yaml for configurable timeouts/concurrency
- Add --persist/--no-persist flags for region/profile persistence
- Persist disabled by default, startup values always loaded
- Replace hardcoded timeout constants with config values
- Add MetricsWindow (15m default) to FileConfig for CloudWatch metrics
- Move lock helpers from internal/sync to internal/config
- Update cloudwatch.go to use config.File().MetricsWindow()
@claude

This comment was marked as resolved.

- Fix data race in Save() by deep copying Startup.Regions slice
- Extract applyStartupConfig() helper for cleaner precedence handling
- Reduce GetStartup() calls from 2 to 1

Addresses PR #79 review feedback.
@claude

This comment was marked as resolved.

@claude
Copy link
Copy Markdown

claude Bot commented Jan 2, 2026

Pull Request Review: Config File Support

Summary

This PR adds comprehensive configuration file support to claws, allowing users to customize timeouts, concurrency limits, CloudWatch windows, and persist profile/region selections. The implementation is well-structured with excellent test coverage (284 lines of new tests).

Code Quality & Best Practices ✅

Strengths:

  1. Excellent separation of concerns: Config logic cleanly separated into file.go, lock.go, and comprehensive tests
  2. Thread-safety: Proper use of sync.RWMutex with helper functions for safe concurrent access
  3. Atomic writes: Config saving uses temp file + rename pattern (file.go:189-209) to prevent corruption
  4. Custom YAML Duration type: Elegant solution for human-readable duration format
  5. Graceful degradation: Missing config file returns defaults rather than errors

Minor Concerns:

1. Silent error handling in File() singleton (file.go:126-134)
If the config file exists but contains malformed YAML, the error is silently ignored. Users won't know their config is broken.
Recommendation: Log the error using the log package for diagnosability.

2. Error handling in persistence
In app.go:336-344 and app.go:366-376, config save errors are logged but don't notify the user. Consider showing a transient error message when persistence fails.

Performance Considerations ⚡

Config save I/O on every region/profile change
Every change triggers a synchronous file write (app.go:342, 373).

Impact: Minimal for most users, but could be noticeable on slow storage.
Recommendations: Consider debouncing saves or making them asynchronous. Current approach is acceptable for MVP.

Security ✅

  • Config file permissions (0755) are acceptable since no secrets are stored
  • Path traversal protection is correct (uses UserHomeDir + hardcoded path)
  • YAML parsing is secure (gopkg.in/yaml.v3 with fixed types)

Consideration: Document that credentials should NEVER be added to this config (only in ~/.aws/credentials)

Test Coverage 📊

Excellent coverage with 284 lines covering:

  • Duration marshal/unmarshal (including invalid values)
  • Default config generation, missing file handling
  • Save/load roundtrip, partial config with defaults
  • Negative value handling, zero value getters

Nice-to-have additions: concurrent access tests, save failure scenarios, malformed YAML handling

Verdict

This is a high-quality PR that adds significant value. The code is well-structured, thread-safe, and thoroughly tested.

Required changes: None

Recommended improvements (post-merge):

  1. Add error logging for malformed config files in File()
  2. Consider user notification for persistence failures
  3. Add concurrent access tests for extra confidence

Recommendation: APPROVE ✅

Great work! The atomic file writes, thread-safe design, and comprehensive test coverage demonstrate strong engineering practices.

@yimsk yimsk merged commit 8fa253e into develop Jan 2, 2026
5 checks passed
@yimsk yimsk deleted the feat/issue-67 branch January 2, 2026 14:04
@yimsk yimsk mentioned this pull request Jan 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant