atmos icon indicating copy to clipboard operation
atmos copied to clipboard

Refactor stack_processor_utils.go template processing to reduce complexity

Open osterman opened this issue 5 months ago • 0 comments

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

osterman avatar Oct 01 '25 16:10 osterman