cloudposse / cloudposse/atmos

Markdown renderer initialization uses stale configuration and passes large struct by value

Open
#1,619 0 comments 0 reactions 1 assignee Claimed by @samtholiya View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.