Fix Mocks for Windows #877
Conversation
|
Important Cloud Posse Engineering Team Review RequiredThis 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 |
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the GitHub Actions workflow file Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
289-289: Consider adding error handling for permission issues.While the OS-specific condition is correct, consider adding error handling to catch and report permission-related issues that might occur during chmod execution.
if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' -run: chmod +x /usr/local/bin/atmos +run: | + if ! chmod +x /usr/local/bin/atmos; then + echo "Failed to set execute permissions on atmos binary" + exit 1 + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
289-289: LGTM! Matrix strategy and naming conventions are well-structured.
The configuration properly handles different OS targets with clear naming conventions and appropriate matrix variables.
Also applies to: 300-302, 306-311
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
304-315: 🛠️ Refactor suggestionConsider consolidating test execution steps.
The current implementation duplicates the test execution step for different OS targets. This could be simplified into a single step.
- - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' - run: | - atmos test - - - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'windows' - run: | - atmos.exe test + - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} + working-directory: examples/${{ matrix.demo-folder }} + run: | + ${{ matrix.flavor.target == 'windows' && 'atmos.exe' || 'atmos' }} testThis consolidation:
- Reduces code duplication
- Maintains the same functionality
- Makes future maintenance easier
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
288-291: Add quotes around PATH addition for better shell safety.While the current implementation works, let's make it more robust against potential shell interpretation issues.
- echo "${{ matrix.flavor.target == 'windows' && 'D:\\a\\_temp' || '/usr/local/bin' }}" >> $GITHUB_PATH + echo "${{ matrix.flavor.target == 'windows' && 'D:\\a\\_temp' || '/usr/local/bin' }}" >> "$GITHUB_PATH"🧰 Tools
🪛 actionlint (1.7.4)
289-289: shellcheck reported issue in this script: SC2086:info:1:90: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
289-289: shellcheck reported issue in this script: SC2086:info:1:90: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/test.yml (2)
282-286: LGTM! Clean OS-specific artifact path handling.
The conditional path setting for Windows vs Unix systems is well implemented.
292-293: LGTM! Proper OS-specific permission handling.
The execute permission step is correctly conditioned for Unix-like systems only.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
305-316: 🛠️ Refactor suggestionConsider consolidating the test execution steps
Warrior, we can simplify this battle formation by combining these steps into a single command.
Apply this strategic maneuver:
- - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' - run: | - atmos test - - - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'windows' - run: | - atmos.exe test + - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} + working-directory: examples/${{ matrix.demo-folder }} + run: | + ${{ matrix.flavor.target == 'windows' && 'atmos.exe' || 'atmos' }} testThis consolidation:
- Reduces duplication
- Maintains the same functionality
- Makes future maintenance easier
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
292-292: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
296-298: Well-crafted conditional execution!
The battle plan is sound - execute permissions are correctly set only for Unix-like systems.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
289-292:⚠️ Potential issueFix matrix property reference and add quotes to prevent globbing
Warrior, this step has the same issues as the one in the build job:
- The condition uses
matrix.flavor.target- The PATH addition needs quotes for safety
Apply this battle-tested fix:
- - name: Add GNU tar to PATH (significantly faster than windows tar) - if: matrix.flavor.target == 'windows' - run: echo "C:\Program Files\Git\usr\bin" >> $GITHUB_PATH + - name: Add GNU tar to PATH (significantly faster than windows tar) + if: matrix.flavor.target == 'windows' + run: echo "C:\Program Files\Git\usr\bin" >> "$GITHUB_PATH"🧰 Tools
🪛 actionlint (1.7.4)
291-291: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
291-291: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
44-44: property "flavor" is not defined in object type {os: string; target: string}
(expression)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
291-291: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
291-291: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
300-300: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
Line range hint 1-24: Well-structured workflow with clear job dependencies
The workflow is well-organized with distinct jobs for different testing scenarios and proper dependencies. The release job correctly depends on all test jobs, ensuring comprehensive testing before releases.
🧰 Tools
🪛 actionlint (1.7.4)
44-44: property "flavor" is not defined in object type {os: string; target: string}
(expression)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/cli_test.go (3)
3-14: Consider replacing "io/ioutil" with "os" or "io" package functions
Read and Write operations in the "io/ioutil" package are deprecated as of Go 1.19. Please consider swapping to the newer "os" or "io" implementations to future-proof the code.Example fix (partial diff):
-import ( - "bytes" - "errors" - "fmt" - "io/ioutil" - "os" - "os/exec" - "regexp" - "testing" - "gopkg.in/yaml.v3" -) +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "os/exec" + "regexp" + "testing" + "gopkg.in/yaml.v3" +)🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
39-52: Gracefully handle file loading errors for improved error messages
While this function does handle errors, consider adding user-friendly context in the returned error messages to guide debugging.
168-184: Suggestion: Add logging or additional context upon file content mismatch
When file content verification fails, it might help developers if the mismatch is logged in more detail, such as showing the expected pattern and the actual lines that did not match.tests/test_cases.yaml (1)
68-77: Skipping certain tests is fine if not applicable
If you decide to enable "atmos docs" in the future, ensure that any required environment or fixture setup is also present.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/cli_test.go(1 hunks)tests/test_cases.yaml(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/cli_test.go
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
🔇 Additional comments (3)
tests/cli_test.go (2)
61-71: Good job on selectively skipping tests
Using conditional logic to skip disabled tests keeps the test suite clean and prevents unintentional runs. This is a neat approach.
99-123: Comprehensive stdout/stderr validation
The code nicely captures both streams and checks them against regex patterns. This approach effectively validates command behavior under various expected conditions.
tests/test_cases.yaml (1)
13-39: Clear and robust help command validation
The test thoroughly enumerates expected CLI commands in stdout. This approach ensures future changes to the help text are quickly detected. Great job!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_cases.yaml (2)
21-38: Consider grouping command checks for better maintainability.The test verifies each command individually. Consider grouping related commands and using a more maintainable structure:
- stdout: - - "\\batlantis\\b" - - "\\baws\\b" - - "\\bcompletion\\b" + stdout: + # Infrastructure commands + - "\\b(atlantis|aws|terraform)\\b" + # Utility commands + - "\\b(completion|help|version)\\b"
41-67: Consider consolidating version test cases.The test cases for
atmos versionandatmos version --checkhave identical expectations. Consider consolidating them into a parameterized test case to reduce duplication.+ - name: atmos version commands + enabled: true + description: "Verify atmos version commands output version details." + workdir: "../examples/demo-stacks" + matrix: + args: + - ["version"] + - ["version", "--check"] + expect: + stdout: + - '👽 Atmos \d+\.\d+\.\d+ on [a-z]+/[a-z0-9]+' + stderr: [] + exit_code: 0
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(2 hunks)tests/test_cases.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
291-291: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
🔇 Additional comments (3)
tests/test_cases.yaml (2)
7-11: Consider using non-zero exit code for error condition.
The test expects a zero exit code when reporting a missing stacks directory. Error conditions typically return non-zero exit codes to indicate failure, which helps automation tools detect issues.
Let's check if this is consistent with other error cases:
68-77: Investigate and document disabled test case.
This test case is disabled and lacks specific expectations. Either:
- Document why it's disabled and what needs to be fixed
- Remove it if it's no longer needed
- Complete it with proper expectations
Let's check for other disabled tests:
.github/workflows/test.yml (1)
319-328: Good addition of Windows binary verification.
The addition of integrity checks for Windows is excellent. It helps diagnose path and execution issues early.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
321-330: Enhance atmos.exe integrity check.The integrity check is good, but could be more thorough.
- name: Check atmos.exe integrity if: matrix.flavor.target == 'windows' shell: pwsh run: | Write-Output "PATH=$Env:PATH" Write-Output "PATHEXT=$Env:PATHEXT" Get-ChildItem "${{ github.workspace }}" + $atmosPath = Join-Path ${{ github.workspace }} "atmos.exe" + $fileInfo = Get-Item $atmosPath + Write-Output "File size: $($fileInfo.Length) bytes" + Write-Output "Last write time: $($fileInfo.LastWriteTime)" - Get-Command "${{ github.workspace }}\atmos.exe" + try { + $cmd = Get-Command $atmosPath -ErrorAction Stop + Write-Output "Command found at: $($cmd.Source)" + } catch { + throw "Failed to locate atmos.exe command: $_" + } .\atmos.exe version
315-319: Consider consolidating test execution steps.The Windows and non-Windows test execution steps could be consolidated using a conditional command.
- - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' - run: | - atmos test - - - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'windows' - shell: pwsh - run: | - ${{ github.workspace }}\atmos.exe test + - name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} + working-directory: examples/${{ matrix.demo-folder }} + shell: ${{ matrix.flavor.target == 'windows' && 'pwsh' || 'bash' }} + run: | + ${{ matrix.flavor.target == 'windows' && '${{ github.workspace }}\atmos.exe' || 'atmos' }} testAlso applies to: 331-337
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
291-291: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
291-291: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
301-301: shellcheck reported issue in this script: SC1009:info:2:1: The mentioned syntax error was in this simple command
(shellcheck)
301-301: shellcheck reported issue in this script: SC1073:error:2:10: Couldn't parse this double quoted string. Fix to allow more checks
(shellcheck)
301-301: shellcheck reported issue in this script: SC1072:error:4:1: Expected end of double quoted string. Fix any mentioned problems and try again
(shellcheck)
307-307: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
osterman
added
no-release
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
43-46:⚠️ Potential issueRemove duplicate step and use PowerShell for Windows path manipulation.
This step is duplicated at lines 289-292. Additionally, using
echowith Windows paths can be problematic.Remove this duplicate step and update the remaining one (lines 289-292) with PowerShell:
- - name: Add GNU tar to PATH (significantly faster than windows tar) - if: matrix.target == 'windows' - run: echo "C:\Program Files\Git\usr\bin" >> $Env:GITHUB_PATH + - name: Add GNU tar to PATH (significantly faster than windows tar) + if: matrix.target == 'windows' + shell: pwsh + run: | + $gitBinPath = "C:\Program Files\Git\usr\bin" + if (Test-Path $gitBinPath) { + $Env:PATH = "$gitBinPath;" + $Env:PATH + echo "PATH=$Env:PATH" >> $Env:GITHUB_ENV + } else { + throw "Git bin directory not found at: $gitBinPath" + }🧰 Tools
🪛 actionlint (1.7.4)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
321-330: Add error handling to integrity check.While the diagnostic commands are comprehensive, adding explicit error handling would make failures more clear.
- name: Check atmos.exe integrity if: matrix.flavor.target == 'windows' shell: pwsh run: | + $ErrorActionPreference = "Stop" Write-Output "PATH=$Env:PATH" Write-Output "PATHEXT=$Env:PATHEXT" Get-ChildItem "${{ github.workspace }}" + if (-not (Test-Path "${{ github.workspace }}\atmos.exe")) { + throw "atmos.exe not found in workspace" + } Get-Command "${{ github.workspace }}\atmos.exe" .\atmos.exe version
331-337: Enhance Windows test execution robustness.The test execution could be more robust with proper error handling and path validation.
- name: Run tests for ${{ matrix.demo-folder }} with ${{ matrix.flavor.target }} working-directory: examples/${{ matrix.demo-folder }} if: matrix.flavor.target == 'windows' shell: pwsh run: | + $ErrorActionPreference = "Stop" + $atmosPath = "${{ github.workspace }}\atmos.exe" + if (-not (Test-Path $atmosPath)) { + throw "atmos.exe not found at: $atmosPath" + } - ${{ github.workspace }}\atmos.exe test + & $atmosPath test
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
291-291: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
291-291: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
301-301: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
307-307: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
293-297: LGTM! Artifact download configuration is correct.
The download path and artifact naming convention are properly configured.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
102-112: Consolidate PATH handling steps for better maintainability.The current implementation has separate steps for Linux/macOS and Windows. Consider consolidating these steps:
- - name: Add build artifacts directory to PATH for linux or macos - if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' - run: | - echo "${{ github.workspace }}" >> $GITHUB_PATH - chmod +x "${{ github.workspace }}/atmos" - - - name: Add build artifacts directory to PATH for windows - if: matrix.flavor.target == 'windows' - run: | - echo "${{ github.workspace }}" >> $Env:GITHUB_PATH + - name: Add build artifacts directory to PATH + shell: bash + run: | + if [[ "${{ matrix.flavor.target }}" == "windows" ]]; then + echo "${{ github.workspace }}" >> $GITHUB_PATH + else + echo "${{ github.workspace }}" >> $GITHUB_PATH + chmod +x "${{ github.workspace }}/atmos" + fi🧰 Tools
🪛 actionlint (1.7.4)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
110-110: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
348-367: Consider consolidating test execution steps.The test execution steps are duplicated for different platforms. Consider using a conditional command:
- - name: Run tests in ${{ matrix.demo-folder }} for ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos' - run: | - atmos test - - - name: Run tests in ${{ matrix.demo-folder }} for ${{ matrix.flavor.target }} - working-directory: examples/${{ matrix.demo-folder }} - if: matrix.flavor.target == 'windows' - shell: pwsh - run: | - atmos test + - name: Run tests in ${{ matrix.demo-folder }} for ${{ matrix.flavor.target }} + working-directory: examples/${{ matrix.demo-folder }} + shell: ${{ matrix.flavor.target == 'windows' && 'pwsh' || 'bash' }} + run: atmos test
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
87-87: property "os" is not defined in object type {flavor: {os: string; target: string}}
(expression)
94-94: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
94-94: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
110-110: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
324-324: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
324-324: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
334-334: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
340-340: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
422-422: input "fail_level" is not defined in action "reviewdog/action-tflint@v1". available inputs are "fail_on_error", "filter_mode", "flags", "github_token", "level", "reporter", "tflint_config", "tflint_init", "tflint_rulesets", "tflint_target_dir", "tflint_version", "working_directory"
(action)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
82-85: LGTM! Well-structured matrix strategy.
The matrix strategy is well-defined with clear OS and target mappings.
118-127: LGTM! Comprehensive Windows executable verification.
The integrity check for Windows is well-implemented with proper debugging information.
43-46: 🛠️ Refactor suggestion
Enhance Windows PATH handling for GNU tar.
The current implementation has potential issues with Windows path handling. Let's make it more robust:
- - name: Add GNU tar to PATH (significantly faster than windows tar)
- if: matrix.target == 'windows'
- run: echo "C:\Program Files\Git\usr\bin" >> $Env:GITHUB_PATH
+ - name: Add GNU tar to PATH (significantly faster than windows tar)
+ if: matrix.target == 'windows'
+ shell: pwsh
+ run: |
+ $gitBinPath = "C:\Program Files\Git\usr\bin"
+ if (Test-Path $gitBinPath) {
+ Write-Output $gitBinPath >> $Env:GITHUB_PATH
+ } else {
+ throw "Git bin directory not found at: $gitBinPath"
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cli_test.go (1)
7-7: Update deprecated package usage.The
io/ioutilpackage has been deprecated since Go 1.19. Consider using theioorospackages instead.-import "io/ioutil" +import "os"Then update the ReadFile calls:
-data, err := ioutil.ReadFile(filePath) +data, err := os.ReadFile(filePath)🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(4 hunks)tests/cli_test.go(1 hunks)tests/test_cases.yaml(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/cli_test.go
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
94-94: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
110-110: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
324-324: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
324-324: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
334-334: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
340-340: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
422-422: input "fail_level" is not defined in action "reviewdog/action-tflint@v1". available inputs are "fail_on_error", "filter_mode", "flags", "github_token", "level", "reporter", "tflint_config", "tflint_init", "tflint_rulesets", "tflint_target_dir", "tflint_version", "working_directory"
(action)
🔇 Additional comments (6)
tests/test_cases.yaml (2)
11-11: Well done on the cross-platform path pattern!
The regex pattern '(build[/\\]atmos|atmos[/\\]atmos)' correctly handles both forward slashes (/) for Unix-like systems and backslashes (\) for Windows paths.
80-81: Consider enabling more test cases for comprehensive Windows testing.
Several test cases are marked as disabled (enabled: false). Given that this PR focuses on fixing mocks for Windows, consider enabling more test cases to ensure thorough testing across platforms.
Also applies to: 90-91
.github/workflows/test.yml (4)
354-362: Well-implemented Windows binary verification!
The Windows binary integrity check is thorough and well-implemented:
- Verifies PATH environment
- Lists directory contents
- Confirms command availability
- Tests version command
422-422:
Fix invalid tflint parameter.
The fail_level parameter is not supported by the tflint action.
- fail_level: error
+ fail_on_error: trueLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
422-422: input "fail_level" is not defined in action "reviewdog/action-tflint@v1". available inputs are "fail_on_error", "filter_mode", "flags", "github_token", "level", "reporter", "tflint_config", "tflint_init", "tflint_rulesets", "tflint_target_dir", "tflint_version", "working_directory"
(action)
43-45: 🛠️ Refactor suggestion
Improve Windows PATH handling with PowerShell.
Using echo with Windows paths can be problematic. Consider using PowerShell for more reliable path handling.
- - name: Add GNU tar to PATH (significantly faster than windows tar)
- if: matrix.target == 'windows'
- run: echo "C:\Program Files\Git\usr\bin" >> $Env:GITHUB_PATH
+ - name: Add GNU tar to PATH (significantly faster than windows tar)
+ if: matrix.target == 'windows'
+ shell: pwsh
+ run: |
+ $gitBinPath = "C:\Program Files\Git\usr\bin"
+ if (Test-Path $gitBinPath) {
+ Write-Output $gitBinPath >> $Env:GITHUB_PATH
+ } else {
+ throw "Git bin directory not found at: $gitBinPath"
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
102-106: 🛠️ Refactor suggestion
Add error handling for Linux/macOS binary.
The current implementation lacks error handling for the atmos binary.
- name: Add build artifacts directory to PATH for linux or macos
if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos'
run: |
- echo "${{ github.workspace }}" >> $GITHUB_PATH
- chmod +x "${{ github.workspace }}/atmos"
+ ATMOS_PATH="${{ github.workspace }}/atmos"
+ if [[ ! -f "$ATMOS_PATH" ]]; then
+ echo "Error: atmos binary not found at $ATMOS_PATH"
+ exit 1
+ fi
+ chmod +x "$ATMOS_PATH"
+ echo "${{ github.workspace }}" >> "$GITHUB_PATH"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/cli_test.go (4)
3-16: Update deprecated package usageThe
io/ioutilpackage has been deprecated since Go 1.19. Let's update to the modern alternatives.Apply this diff:
- "io/ioutil" + "os"Then update the following function calls:
- Replace
ioutil.ReadFile()withos.ReadFile()🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
18-39: Add godoc comments for exported typesThese are well-structured types, but as they're exported, they should have proper documentation comments.
Add documentation like this:
+// Expectation defines the expected outcomes for a test case type Expectation struct { +// TestCase represents a single CLI test scenario type TestCase struct { +// TestSuite is a collection of test cases type TestSuite struct {
69-79: Improve error handling in Prepend methodThe error from
filepath.Absis only logged but might affect test reliability.Consider this improvement:
func (pm *PathManager) Prepend(dirs ...string) { + var resolvedPaths []string for _, dir := range dirs { absPath, err := filepath.Abs(dir) if err != nil { - fmt.Printf("Failed to resolve absolute path for %q: %v\n", dir, err) - continue + // Either return error or use the original path as fallback + resolvedPaths = append(resolvedPaths, dir) + continue } - pm.Prepended = append(pm.Prepended, absPath) + resolvedPaths = append(resolvedPaths, absPath) } + pm.Prepended = append(pm.Prepended, resolvedPaths...) }
197-213: Consider platform-specific line endings in output verificationWhen verifying output with regex patterns, we should account for different line endings (CRLF on Windows vs LF on Unix).
Consider normalizing line endings before comparison:
func verifyOutput(t *testing.T, outputType, output string, patterns []string) bool { + // Normalize line endings + output = strings.ReplaceAll(output, "\r\n", "\n") success := true for _, pattern := range patterns {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/cli_test.go
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
🔇 Additional comments (2)
tests/cli_test.go (2)
1-250: Overall implementation looks solid! 💪
The test framework effectively addresses the PR objectives for Windows compatibility while maintaining cross-platform support. The implementation includes proper cleanup, good error reporting, and comprehensive verification of CLI behavior.
🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
139-143: Enhance binary path resolution for Windows compatibility
Since this PR focuses on Windows compatibility, let's ensure the binary path resolution works correctly on Windows.
Run this script to verify binary resolution across platforms:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_cases.yaml (1)
5-5: Standardize path separators for cross-platform compatibilityThe workdir paths use forward slashes which, while they work on Windows, should be made consistent with the rest of the Windows-specific changes.
Consider using a path separator variable or function to handle this consistently across platforms.
Also applies to: 32-32, 103-103, 143-143, 160-160
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_cases.yaml(1 hunks)
🔇 Additional comments (1)
tests/test_cases.yaml (1)
88-99: Re-evaluate disabled tests for Windows compatibility
Two test cases are currently disabled (atmos docs and atmos terraform non-existent). Given that this PR focuses on Windows compatibility, these tests should be evaluated and enabled if they're important for cross-platform validation.
Please verify if these tests should remain disabled or if they need to be updated for Windows compatibility.
Also applies to: 127-139
| exit_code: 0 | ||
|
|
||
| - name: atmos version --check | ||
| enabled: true | ||
| description: "Verify atmos version --check command functions correctly." | ||
| workdir: "../examples/demo-stacks" | ||
| command: "atmos" | ||
| args: | ||
| - "version" | ||
| - "--check" | ||
| expect: | ||
| stdout: | ||
| - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9]+' | ||
| stderr: | ||
| - "^$" | ||
| exit_code: 0 | ||
|
|
||
| - name: atmos docs | ||
| enabled: false | ||
| description: "Ensure atmos docs command executes without errors." | ||
| workdir: "../" | ||
| command: "atmos" | ||
| args: | ||
| - "docs" | ||
| expect: | ||
| exit_code: 0 | ||
| stderr: | ||
| - "^$" | ||
|
|
||
| - name: atmos docs myapp | ||
| enabled: true | ||
| description: "Validate atmos docs command outputs documentation for a specific component." | ||
| workdir: "../examples/demo-stacks/" | ||
| command: "atmos" | ||
| args: | ||
| - "docs" | ||
| - "myapp" | ||
| expect: | ||
| stdout: | ||
| - "Example Terraform Weather Component" | ||
| stderr: | ||
| - "^$" | ||
| exit_code: 0 | ||
|
|
||
| - name: atmos non-existent | ||
| enabled: true | ||
| description: "Ensure atmos CLI returns an error for a non-existent command." | ||
| workdir: "../" | ||
| command: "atmos" | ||
| args: | ||
| - "non-existent" | ||
| expect: | ||
| stderr: | ||
| - 'unknown command "non-existent" for "atmos"' | ||
| exit_code: 1 | ||
|
|
||
| - name: atmos terraform non-existent | ||
| enabled: false | ||
| description: "Ensure atmos CLI returns an error for a non-existent command." | ||
| workdir: "../" | ||
| command: "atmos" | ||
| args: | ||
| - "terraform" | ||
| - "non-existent" | ||
| expect: | ||
| stderr: | ||
| - 'unknown command "non-existent" for "atmos"' | ||
| exit_code: 1 | ||
|
|
||
| - name: atmos describe config -f yaml | ||
| enabled: true | ||
| description: "Ensure atmos CLI outputs the Atmos configuration in YAML." | ||
| workdir: "../examples/demo-stacks/" | ||
| command: "atmos" | ||
| args: | ||
| - "describe" | ||
| - "config" | ||
| - "-f" | ||
| - "yaml" | ||
| expect: | ||
| stdout: | ||
| - 'append_user_agent: Atmos/(\d+\.\d+\.\d+|test) \(Cloud Posse; \+https:\/\/atmos\.tools\)' | ||
| stderr: | ||
| - "^$" | ||
| exit_code: 0 | ||
|
|
||
| - name: atmos describe config | ||
| enabled: true | ||
| description: "Ensure atmos CLI outputs the Atmos configuration in JSON." | ||
| workdir: "../examples/demo-stacks/" | ||
| command: "atmos" | ||
| args: | ||
| - "describe" | ||
| - "config" | ||
| expect: | ||
| stdout: | ||
| - '"append_user_agent": "Atmos/(\d+\.\d+\.\d+|test) \(Cloud Posse; \+https:\/\/atmos\.tools\)"' | ||
| stderr: | ||
| - "^$" | ||
| exit_code: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Windows-specific test cases
Given the PR's focus on Windows compatibility, consider adding the following test cases:
- Long path handling (Windows has a 260-character path limit by default)
- UNC path handling (Windows network paths)
- Drive letter handling (e.g., C:\ paths)
| - "version" | ||
| expect: | ||
| stdout: | ||
| - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9]+' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance platform string pattern for Windows
The current regex pattern [a-z]+/[a-z0-9]+ might not handle Windows platform strings correctly. Windows typically uses strings like "windows/amd64".
- - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9]+'
+ - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9_]+' 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9]+' | |
| - '👽 Atmos (\d+\.\d+\.\d+|test) on [a-z]+/[a-z0-9_]+' |
| - name: "which atmos" | ||
| enabled: true | ||
| description: "Ensure atmos CLI is installed and we're using the one that was built." | ||
| workdir: "../" | ||
| command: "which" | ||
| args: | ||
| - "atmos" | ||
| expect: | ||
| stdout: | ||
| - '(build[/\\]atmos|atmos[/\\]atmos)' | ||
| stderr: | ||
| - "^$" | ||
| exit_code: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Windows command alternative for "which"
The which command is Unix-specific. For Windows compatibility, consider using where command or implementing OS-specific command selection.
- name: "which atmos"
enabled: true
description: "Ensure atmos CLI is installed and we're using the one that was built."
workdir: "../"
- command: "which"
+ command: "{{.OsCommand}}" # Template variable to be replaced with "which" or "where"
args:
- "atmos"Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
319-341: Consider creating a reusable workflow, warrior!The PATH setup and binary validation steps are duplicated across jobs. Consider extracting these into a reusable workflow.
Create a new file
.github/workflows/setup-atmos.yml:name: Setup Atmos description: Sets up Atmos binary and validates its integrity inputs: target: description: Target OS (linux, windows, macos) required: true runs: using: composite steps: - name: Add GNU tar to PATH if: inputs.target == 'windows' shell: pwsh run: | $gitBinPath = "C:\Program Files\Git\usr\bin" if (Test-Path $gitBinPath) { Write-Output $gitBinPath >> $Env:GITHUB_PATH } else { throw "Git bin directory not found at: $gitBinPath" } - name: Download build artifacts uses: actions/download-artifact@v4 with: name: build-artifacts-${{ inputs.target }} path: ${{ github.workspace }} - name: Setup for Linux/macOS if: inputs.target == 'linux' || inputs.target == 'macos' shell: bash run: | ATMOS_PATH="${{ github.workspace }}/atmos" if [[ ! -f "$ATMOS_PATH" ]]; then echo "Error: atmos binary not found at $ATMOS_PATH" exit 1 fi chmod +x "$ATMOS_PATH" echo "${{ github.workspace }}" >> "$GITHUB_PATH" - name: Setup for Windows if: inputs.target == 'windows' shell: pwsh run: | $atmosPath = Join-Path ${{ github.workspace }} "atmos.exe" if (-not (Test-Path $atmosPath)) { throw "atmos.exe not found at: $atmosPath" } echo "${{ github.workspace }}" >> $Env:GITHUB_PATHThen use it in your jobs:
steps: - uses: ./.github/workflows/setup-atmos with: target: ${{ matrix.flavor.target }}🧰 Tools
🪛 actionlint (1.7.4)
324-324: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
324-324: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
334-334: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
340-340: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
tests/test_cases.yaml (1)
1-170: Add Windows-specific test cases, warrior!For better Windows compatibility testing, consider adding these test cases:
- name: "atmos windows paths" enabled: true description: "Verify atmos CLI handles Windows paths correctly" workdir: "../examples/demo-stacks" command: "atmos" args: - "describe" - "config" expect: stdout: - "C:\\Program Files\\atmos" stderr: - "^$" exit_code: 0 - name: "atmos unc paths" enabled: true description: "Verify atmos CLI handles UNC paths" workdir: "../examples/demo-stacks" command: "atmos" args: - "describe" - "config" expect: stdout: - "\\\\server\\share\\path" stderr: - "^$" exit_code: 0
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(4 hunks)tests/cli_test.go(1 hunks)tests/test_cases.yaml(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tests/cli_test.go
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
94-94: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
110-110: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
324-324: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
324-324: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
334-334: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
340-340: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
422-422: input "fail_level" is not defined in action "reviewdog/action-tflint@v1". available inputs are "fail_on_error", "filter_mode", "flags", "github_token", "level", "reporter", "tflint_config", "tflint_init", "tflint_rulesets", "tflint_target_dir", "tflint_version", "working_directory"
(action)
🔇 Additional comments (4)
.github/workflows/test.yml (4)
82-85: Well-structured matrix strategy, warrior!
The matrix configuration clearly defines the OS and target combinations, making it easier to maintain and extend.
422-422:
Fix tflint action parameter, warrior!
The fail_level parameter is not supported by the tflint action. Use fail_on_error instead.
- fail_level: error
+ fail_on_error: trueLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
422-422: input "fail_level" is not defined in action "reviewdog/action-tflint@v1". available inputs are "fail_on_error", "filter_mode", "flags", "github_token", "level", "reporter", "tflint_config", "tflint_init", "tflint_rulesets", "tflint_target_dir", "tflint_version", "working_directory"
(action)
102-106: 🛠️ Refactor suggestion
Add binary validation for Linux/macOS, warrior!
Let's validate the binary existence and add proper error handling.
- name: Add build artifacts directory to PATH for linux or macos
if: matrix.flavor.target == 'linux' || matrix.flavor.target == 'macos'
run: |
+ ATMOS_PATH="${{ github.workspace }}/atmos"
+ if [[ ! -f "$ATMOS_PATH" ]]; then
+ echo "Error: atmos binary not found at $ATMOS_PATH"
+ exit 1
+ fi
+ chmod +x "$ATMOS_PATH"
echo "${{ github.workspace }}" >> "$GITHUB_PATH"
- chmod +x "${{ github.workspace }}/atmos"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
104-104: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
43-45: 🛠️ Refactor suggestion
Use PowerShell for Windows path handling, warrior!
The current approach using echo with Windows paths could lead to path interpretation issues. Let's make it more robust with PowerShell.
- - name: Add GNU tar to PATH (significantly faster than windows tar)
- if: matrix.target == 'windows'
- run: echo "C:\Program Files\Git\usr\bin" >> $Env:GITHUB_PATH
+ - name: Add GNU tar to PATH (significantly faster than windows tar)
+ if: matrix.target == 'windows'
+ shell: pwsh
+ run: |
+ $gitBinPath = "C:\Program Files\Git\usr\bin"
+ if (Test-Path $gitBinPath) {
+ Write-Output $gitBinPath >> $Env:GITHUB_PATH
+ } else {
+ throw "Git bin directory not found at: $gitBinPath"
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
45-45: shellcheck reported issue in this script: SC2028:info:1:6: echo may not expand escape sequences. Use printf
(shellcheck)
45-45: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
| "bytes" | ||
| "errors" | ||
| "fmt" | ||
| "io/ioutil" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated package usage, warrior!
The io/ioutil package is deprecated since Go 1.19. Let's use the os package instead.
-import (
- "io/ioutil"
+import (
+ "os"Then update the ReadFile calls:
- data, err := ioutil.ReadFile(filePath)
+ data, err := os.ReadFile(filePath)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
what
why
atmos version)Summary by CodeRabbit
Summary by CodeRabbit
New Features
atmosCLI, including error handling and command outputs.Bug Fixes
Chores