cloudposse / cloudposse/atmos

Refactor stack_processor_utils.go template processing to reduce complexity

Closed
#1,575 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
1.4k
Forks
176
Avg merge
2d 25m
Merged PRs (30d)
143

Description

Describe the Bug

The template processing logic in internal/exec/stack_processor_utils.go:279 has high complexity (nestif: 6) and required a //nolint:nestif directive to pass linting. This indicates the function has deeply nested conditional logic that should be refactored.

Current State

File: internal/exec/stack_processor_utils.go
Line: 279
Function: Template processing in stack manifest imports
Complexity: 6 (nestif)

//nolint:nestif // Acceptable complexity for error handling with merge context
if !skipTemplatesProcessingInImports && len(context) > 0 {
    stackManifestTemplatesProcessed, err = ProcessTmpl(relativeFilePath, stackYamlConfig, context, ignoreMissingTemplateValues)
    if err != nil {
        if atmosConfig.Logs.Level == u.LogLevelTrace || atmosConfig.Logs.Level == u.LogLevelDebug {
            stackManifestTemplatesErrorMessage = fmt.Sprintf("\n\n%s", stackYamlConfig)
        }
        // Check if we have merge context to provide enhanced error formatting
        if mergeContext != nil {
            // Wrap the error with the sentinel first to preserve it
            wrappedErr := fmt.Errorf("%w: %v", errUtils.ErrInvalidStackManifest, err)
            // Then format it with context information
            e := mergeContext.FormatError(wrappedErr, fmt.Sprintf("stack manifest '%s'%s", relativeFilePath, stackManifestTemplatesErrorMessage))
            return nil, nil, nil, nil, nil, nil, nil, e
        } else {
            e := fmt.Errorf("%w: stack manifest '%s'\n%v%s", errUtils.ErrInvalidStackManifest, relativeFilePath, err, stackManifestTemplatesErrorMessage)
            return nil, nil, nil, nil, nil, nil, nil, e
        }
    }
}

Why This Needs Refactoring

  1. Testability: Deeply nested logic is harder to unit test in isolation
  2. Maintainability: Complex conditionals make future modifications error-prone
  3. Readability: Multiple levels of nesting reduce code clarity
  4. Code Quality: Linter warnings indicate code smell that should be addressed

Proposed Solution

Refactor the template processing logic into smaller, focused functions:

Example Approach
// processStackManifestTemplate processes Go templates in the imported stack manifest.
func processStackManifestTemplate(
    atmosConfig *schema.AtmosConfiguration,
    relativeFilePath string,
    stackYamlConfig string,
    context map[string]any,
    ignoreMissingTemplateValues bool,
    mergeContext *MergeContext,
) (string, error) {
    processed, err := ProcessTmpl(relativeFilePath, stackYamlConfig, context, ignoreMissingTemplateValues)
    if err != nil {
        return "", formatTemplateError(atmosConfig, relativeFilePath, stackYamlConfig, err, mergeContext)
    }
    return processed, nil
}

// formatTemplateError formats template processing errors with appropriate context.
func formatTemplateError(
    atmosConfig *schema.AtmosConfiguration,
    relativeFilePath string,
    stackYamlConfig string,
    err error,
    mergeContext *MergeContext,
) error {
    errorMessage := buildTemplateErrorMessage(atmosConfig, stackYamlConfig)
    
    if mergeContext != nil {
        return formatErrorWithMergeContext(mergeContext, relativeFilePath, errorMessage, err)
    }
    return formatErrorWithoutMergeContext(relativeFilePath, errorMessage, err)
}

// buildTemplateErrorMessage constructs the error message based on log level.
func buildTemplateErrorMessage(atmosConfig *schema.AtmosConfiguration, stackYamlConfig string) string {
    if atmosConfig.Logs.Level == u.LogLevelTrace || atmosConfig.Logs.Level == u.LogLevelDebug {
        return fmt.Sprintf("\n\n%s", stackYamlConfig)
    }
    return ""
}

// formatErrorWithMergeContext formats error using merge context.
func formatErrorWithMergeContext(
    mergeContext *MergeContext,
    relativeFilePath string,
    errorMessage string,
    err error,
) error {
    wrappedErr := fmt.Errorf("%w: %v", errUtils.ErrInvalidStackManifest, err)
    return mergeContext.FormatError(wrappedErr, fmt.Sprintf("stack manifest '%s'%s", relativeFilePath, errorMessage))
}

// formatErrorWithoutMergeContext formats error without merge context.
func formatErrorWithoutMergeContext(
    relativeFilePath string,
    errorMessage string,
    err error,
) error {
    return fmt.Errorf("%w: stack manifest '%s'\n%v%s", errUtils.ErrInvalidStackManifest, relativeFilePath, err, errorMessage)
}

Acceptance Criteria

  • Remove //nolint:nestif directive from line 279
  • Refactor template processing into smaller, single-purpose functions
  • Each extracted function should have a clear, testable responsibility
  • Add unit tests for each new helper function
  • Ensure existing tests continue to pass
  • golangci-lint passes without suppressions
  • Code coverage maintained or improved

Additional Context

This issue was identified during PR #1504 when adding Windows CI path normalization fixes. The nolint directive was added as a temporary measure to unblock the PR, but the underlying complexity issue should be addressed.

Related

  • Commit: 7ba97f332
  • File: internal/exec/stack_processor_utils.go
  • Lines: 279-297

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read the template-processing logic around line 279 in internal/exec/stack_processor_utils.go, including ProcessTmpl, error formatting, and the existing merge-context behavior. Identify the focused helpers and tests needed to cover debug/trace output, merge and non-merge errors, and successful processing. Done means the nolint directive is removed, helper tests are added, existing tests pass, and golangci-lint passes without suppressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.