Skip to content

Add tests for !terraform.output. Improve logging and error handling #1235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 37 commits into from
May 7, 2025

Conversation

aknysh
Copy link
Member

@aknysh aknysh commented May 4, 2025

what

  • Add tests for !terraform.output
  • Refactor code
  • Improve logging and error handling
  • Install OpenTofu in the GitHub actions for testing Atmos OpenTofu components

why

  • Add more unit tests for the !terraform.output YAML function. Test !terraform.output for OpenTofu tofu command per component
  • Refactor code: move all shell related functions to pkg/utils/shell_utils.go
  • Replace the deprecated logging functions with github.com/charmbracelet/log
  • Use structured errors for error handling

@aknysh aknysh added the no-release Do not create a new release (wait for additional code changes) label May 4, 2025
@aknysh aknysh self-assigned this May 4, 2025
@github-actions github-actions bot added the size/l label May 4, 2025
Copy link

mergify bot commented May 4, 2025

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label May 4, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes May 5, 2025
Copy link

codecov bot commented May 6, 2025

Codecov Report

Attention: Patch coverage is 47.71574% with 103 lines in your changes missing coverage. Please review.

Project coverage is 33.42%. Comparing base (c87cd51) to head (016fd7c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/utils/shell_utils.go 30.90% 37 Missing and 1 partial ⚠️
internal/exec/terraform_outputs.go 59.77% 35 Missing ⚠️
internal/exec/terraform.go 43.47% 26 Missing ⚠️
internal/exec/yaml_func_terraform_output.go 71.42% 2 Missing ⚠️
pkg/utils/yaml_func_exec.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1235      +/-   ##
==========================================
+ Coverage   32.89%   33.42%   +0.52%     
==========================================
  Files         223      224       +1     
  Lines       23791    23848      +57     
==========================================
+ Hits         7827     7972     +145     
+ Misses      14798    14703      -95     
- Partials     1166     1173       +7     
Flag Coverage Δ
unittests 33.42% <47.71%> (+0.52%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aknysh aknysh requested a review from osterman May 6, 2025 03:48
@aknysh
Copy link
Member Author

aknysh commented May 6, 2025

@coderabbitai please review

Copy link
Contributor

coderabbitai bot commented May 6, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get PR details
gh pr view 1235 --json title,body,baseRefName,headRefName

# Get a list of changed files
gh pr view 1235 --json files --jq '.files[].path'

# Get the diff for the PR
gh pr diff 1235

Length of output: 74057


@aknysh I've completed a review of this PR. Here's what I found:

Summary of Changes

This PR focuses on three main areas:

  1. Testing Improvements:

    • Added tests for the !terraform.output YAML function
    • Added OpenTofu support in CI workflows to test compatibility
  2. Code Refactoring:

    • Moved shell-related utilities to pkg/utils/shell_utils.go
    • Improved code organization and reduced duplication
  3. Logging and Error Handling:

    • Replaced deprecated logging with structured logging from github.com/charmbracelet/log
    • Implemented structured errors with descriptive messages
    • Enhanced error context in responses

Notable Improvements

  • Better Error Handling: Created defined error variables at the package level like ErrHTTPBackendWorkspaces, ErrMissingStack, etc., making the code more maintainable
  • Structured Logging: Replaced ad-hoc logging with structured logging, providing clearer context
  • Test Coverage: Added comprehensive testing for the !terraform.output YAML function
  • Multi-vendor Support: Added OpenTofu installation in GitHub Actions to ensure compatibility

Areas for Improvement

  • While this PR adds valuable tests, the overall coverage remains somewhat low (~38%). Consider adding more tests in future PRs to improve this further.

Overall, this PR represents a significant improvement to the codebase with better organization, error handling, and testing. These changes will make the code more maintainable and robust.

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When updating logging to use the semantic logger, make sure to also use semantic logging.

Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
coderabbitai[bot]
coderabbitai bot previously approved these changes May 7, 2025
@aknysh aknysh requested a review from osterman May 7, 2025 01:52
Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can dismiss the lint warnings

@aknysh aknysh merged commit d4fa2fe into main May 7, 2025
51 checks passed
@aknysh aknysh deleted the update-terraform-output-4 branch May 7, 2025 02:28
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-release Do not create a new release (wait for additional code changes) size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants