Markdown renderer initialization uses stale configuration and passes large struct by value
- Dominant language
- Go
- Stars
- 1.4k
- Forks
- 175
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 134
Description
# Markdown Renderer Initialization Uses Stale Configuration
## Problem
The markdown renderer is initialized once in `cmd/root.go:Execute()` with a configuration that doesn't include CLI flag overrides. This causes two issues:
1. **Inefficient**: `utils.InitializeMarkdown()` currently accepts `AtmosConfiguration` by value, creating expensive copies of a large struct (violates Go best practices and likely triggers linting warnings)
2. **Stale config**: Commands that run later use a different configuration (`tmpConfig` in `PersistentPreRun`) that respects CLI flags like `--base-path`, `--config`, `--config-path`, but the markdown renderer still has the old config
## Current Code Flow
```go
// cmd/root.go:Execute()
func Execute() error {
// Load config without CLI flags
atmosConfig, initErr = cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)
// Initialize markdown renderers (happens once, early)
utils.InitializeMarkdown(atmosConfig) // ❌ Pass by value (expensive copy)
errUtils.InitializeMarkdown(&atmosConfig) // ✅ Pass by pointer
// ... later commands execute
}
// cmd/root.go:PersistentPreRun
PersistentPreRun: func(cmd *cobra.Command, args []string) {
// Load config WITH CLI flags (different from above!)
configAndStacksInfo := schema.ConfigAndStacksInfo{
AtmosBasePath: bp, // from --base-path
AtmosConfigFilesFromArg: cfgFiles, // from --config
AtmosConfigDirsFromArg: cfgDirs, // from --config-path
}
tmpConfig, err := cfg.InitCliConfig(configAndStacksInfo, false)
// ❌ Markdown renderer still uses old atmosConfig, not tmpConfig!
}
```
## Impact
- Markdown renderer doesn't respect CLI flag overrides for width, color profiles, etc.
- Passing `AtmosConfiguration` by value creates unnecessary copies of a large struct
- Could cause subtle bugs where markdown rendering behaves differently than expected
## Proposed Solution
Implement **two-stage initialization** with proper pointer usage:
### Stage 1: Early Initialization (for early errors)
```go
// cmd/root.go:Execute()
atmosConfig, initErr = cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)
// Initialize for early errors (before PersistentPreRun)
utils.InitializeMarkdown(&atmosConfig) // ✅ Pass by pointer
errUtils.InitializeMarkdown(&atmosConfig)
```
### Stage 2: Re-initialize with CLI-aware Config
```go
// cmd/root.go:PersistentPreRun
tmpConfig, err := cfg.InitCliConfig(configAndStacksInfo, false)
// Re-initialize with CLI-aware config
utils.InitializeMarkdown(&tmpConfig) // ✅ Pass by pointer
errUtils.InitializeMarkdown(&tmpConfig)
```
### Implementation Changes Required
1. **Update `utils.InitializeMarkdown` signature**:
```go
// pkg/utils/markdown_utils.go
func InitializeMarkdown(atmosConfig *schema.AtmosConfiguration) {
defer perf.Track(atmosConfig, "utils.InitializeMarkdown")()
var err error
render, err = markdown.NewTerminalMarkdownRenderer(atmosConfig)
// ...
}
```
2. **Update `markdown.NewTerminalMarkdownRenderer` signature**:
```go
// pkg/ui/markdown/renderer.go
func NewTerminalMarkdownRenderer(atmosConfig *schema.AtmosConfiguration) (*Renderer, error) {
return NewRenderer(atmosConfig, ...)
}
```
3. **Update `markdown.NewRenderer` signature**:
```go
// pkg/ui/markdown/renderer.go
func NewRenderer(atmosConfig *schema.AtmosConfiguration, opts ...Option) (*Renderer, error) {
r := &Renderer{
atmosConfig: atmosConfig, // Store pointer directly
// ...
}
}
```
4. **Make initialization idempotent**: Allow `InitializeMarkdown` to be called multiple times safely (already works, just document it)
5. **Update all test calls** from `InitializeMarkdown(config)` to `InitializeMarkdown(&config)`
## Benefits
- ✅ **Efficient**: No expensive struct copies, passes large struct by pointer
- ✅ **Correct config**: Markdown renderer uses CLI-flag-aware configuration
- ✅ **Early errors**: Still get markdown rendering for early errors (stage 1)
- ✅ **Consistent**: Both `utils` and `errUtils` use pointer semantics
- ✅ **Compliant**: Follows Go best practices for passing large structs
## Testing
Verify that:
1. Early errors (before PersistentPreRun) still render with markdown
2. Commands respect `--base-path`, `--config`, `--config-path` flags in markdown output
3. Markdown width/color settings from config are applied correctly
4. All existing tests pass
5. No linting warnings about passing large structs by value
## Related
- This issue was discovered during PR #1599 (error handling implementation)
- Temporary fix: Reverted to pass-by-value in commit d8b236047 to unblock CI
- Long-term fix tracked here
Contributor guide
Assessment
This issue has not been assessed yet.