Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough""" WalkthroughIntegration test jobs and related reusable commands were removed from the CircleCI configuration. Two new shell scripts were added for mock generation and provider cache setup. The GitHub Actions workflows for base and integration tests were updated: test output is now visible in logs, the integration test matrix was expanded and renamed, and additional resource cleanup steps were introduced. Minor test improvements were made for AWS and Tflint integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant TestRunner
participant SetupScript
GitHubActions->>SetupScript: Run provider-cache.sh (if required)
GitHubActions->>SetupScript: Run generate-mocks.sh (if required)
GitHubActions->>TestRunner: Run go test | tee | go-junit-report
TestRunner->>GitHubActions: Output test results (visible in logs and as JUnit XML)
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
.github/scripts/setup/generate-secrets.sh (1)
48-53: Remove duplicate AWS export branches
AWS secrets are exported twice: once at lines 31–34 and again at 48–53. This duplication increases maintenance overhead and risks drift. Consolidate the AWS export logic into a single branch (for example, extend the first AWS block to also handleAWS_TEST_S3_ASSUME_ROLE) and remove the redundant lines below.Proposed diff to remove the duplicate block:
- elif [[ "$SECRET" == "AWS_ACCESS_KEY_ID" && -n "${AWS_ACCESS_KEY_ID}" ]]; then - printf "export AWS_ACCESS_KEY_ID='%s'\n" "${AWS_ACCESS_KEY_ID}" >> "$ENV_FILE" - elif [[ "$SECRET" == "AWS_SECRET_ACCESS_KEY" && -n "${AWS_SECRET_ACCESS_KEY}" ]]; then - printf "export AWS_SECRET_ACCESS_KEY='%s'\n" "${AWS_SECRET_ACCESS_KEY}" >> "$ENV_FILE" - elif [[ "$SECRET" == "AWS_TEST_S3_ASSUME_ROLE" && -n "${AWS_TEST_S3_ASSUME_ROLE}" ]]; then - printf "export AWS_TEST_S3_ASSUME_ROLE='%s'\n" "${AWS_TEST_S3_ASSUME_ROLE}" >> "$ENV_FILE"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/setup/generate-secrets.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Test (Tflint)
- GitHub Check: Test (SSH)
- GitHub Check: Test (Windows)
- GitHub Check: Test (AWS Tofu)
- GitHub Check: Test (Provider Cache Tofu)
- GitHub Check: Test (AWS Terraform 1.5)
- GitHub Check: Test (SOPS)
- GitHub Check: Test (Fixtures OpenTofu 1.9)
- GitHub Check: Test (Fixtures Terraform 1.11)
- GitHub Check: Test (Fixtures Terraform 1.5)
- GitHub Check: Test (macos)
- GitHub Check: Build (windows/amd64)
- GitHub Check: Build (windows/386)
- GitHub Check: Build (darwin/arm64)
- GitHub Check: Build (linux/arm64)
- GitHub Check: Build (linux/amd64)
- GitHub Check: Test (ubuntu)
- GitHub Check: Build (linux/386)
- GitHub Check: Build (darwin/amd64)
- GitHub Check: lint
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (1)
.github/scripts/setup/generate-secrets.sh (1)
11-13: Ensure AWS variables should be globally mandatory
The script now unconditionally enforcesAWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY, andAWS_TEST_S3_ASSUME_ROLEeven when those secrets may not be needed for non-AWS integration runs. This change will cause failures in GCP-only or other workflows that do not set these variables. Consider guarding these: "${VAR:?…}"checks behind theSECRETSlist or the specific integration context so that AWS credentials are only required when actually exporting AWS-related secrets.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.github/workflows/integration-test.yml (6)
64-71: Reduce duplication in AWS Tofu job definitions
The AWS Tofu job duplicates the sametags,run,secrets, andsetup_scriptspatterns repeated for other AWS variants. Consider extracting common AWS test parameters (e.g., via a matrix variable liketf_version) or using YAML anchors & aliases to DRY up these blocks.
72-79: DRY up Terraform 1.5 AWS job
This block is nearly identical to the AWS Tofu definition except for thesetup_scripts. To improve maintainability, consider parameterizing the Terraform version within the matrix (e.g.,tf_version: ["tofu","1.5","1.11"]) and referencing a single AWS job configuration.
80-87: Parameterize AWS Terraform 1.11 in integration matrix
Same pattern repeated here; extracting the Terraform version into a matrix variable would reduce copy-paste and ease future version bumps.
110-115: Combine provider cache jobs with version matrix
The “Provider Cache Terraform 1.11” job could be merged with the Tofu variant using a version matrix similar to the AWS jobs, consolidating repeatedsetup_scriptsfor provider cache.
116-121: Consolidate Provider Cache Tofu configuration
This block mirrors the Terraform 1.11 provider cache job. Consider creating a matrix entry forruntime: ["terraform-1-11","tofu"]and looping over both in one job.
244-244: Add missing newline at end of file
YAML files should end with a newline to satisfy linting and improve compatibility.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 244-244: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/base-test.yml(1 hunks).github/workflows/integration-test.yml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/base-test.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/integration-test.yml
[error] 244-244: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test (Windows)
- GitHub Check: lint
- GitHub Check: Test (ubuntu)
- GitHub Check: Test (macos)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (7)
.github/workflows/integration-test.yml (7)
20-23: Integration matrix expansion looks good
Adding “Fixtures OpenTofu 1.9” to the matrix ensures coverage of the new OpenTofu runtime.
122-126: Deprecated tests re-enabled
The “Deprecated” matrix entry cleanly reinstates the tests. Looks good—no further action needed.
127-133: Mock tests execution defined correctly
Mock generation and execution steps are properly configured and reference the newgenerate-mocks.shscript.
134-139: Race and Parse tests integrated
Thetest_argsparameter for the Race job is now consumed in thego testcommand, and the Parse job is correctly defined. This ensures the race detector flag is passed.
148-154: Disk cleanup step looks safe
Removing unused tool directories and pruning Docker artifacts is a good strategy for freeing up space on non-Windows runners.
229-229: Injecting TEST_ARGS into environment
TheTEST_ARGSenvironment variable is now set from the matrix and picked up by the test invocation. Great for flexible flagging.
237-243: JUnit report display configured correctly
The addition ofdetailed_summary,include_time_in_summary, andgroup_suiteflags provides a comprehensive test overview.
| - name: Upload Report (${{ matrix.integration.name }})) | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: test-report-${{ matrix.integration.name }} | ||
| path: result.xml | ||
|
|
There was a problem hiding this comment.
Typo: extra parenthesis in Upload Report name
There's an extra ) in the step name which may cause YAML parsing issues:
- - name: Upload Report (${{ matrix.integration.name }}))
+ - name: Upload Report (${{ matrix.integration.name }})📝 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.
| - name: Upload Report (${{ matrix.integration.name }})) | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: test-report-${{ matrix.integration.name }} | |
| path: result.xml | |
| - name: Upload Report (${{ matrix.integration.name }}) | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: test-report-${{ matrix.integration.name }} | |
| path: result.xml |
🤖 Prompt for AI Agents
In .github/workflows/integration-test.yml around lines 231 to 236, remove the
extra closing parenthesis in the step name "Upload Report (${{
matrix.integration.name }}))" so it reads "Upload Report (${{
matrix.integration.name }})" to fix the YAML syntax error.
| # print command arguments | ||
| set -x | ||
| go test -v -timeout 45m ${TAGS:+-tags "$TAGS"} ${RUN:+-run "$RUN"} ${TEST_ARGS} "${TARGET}" | tee >(go-junit-report -set-exit-code > result.xml) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve failure handling and output verbosity
Using set -x aids debugging, but consider enabling pipefail to ensure the workflow fails if any part of the pipeline (including go-junit-report) errors out. E.g., add:
- set -x
+ set -eo pipefail && set -x📝 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.
| # print command arguments | |
| set -x | |
| go test -v -timeout 45m ${TAGS:+-tags "$TAGS"} ${RUN:+-run "$RUN"} ${TEST_ARGS} "${TARGET}" | tee >(go-junit-report -set-exit-code > result.xml) | |
| # print command arguments | |
| - set -x | |
| + set -eo pipefail && set -x | |
| go test -v -timeout 45m ${TAGS:+-tags "$TAGS"} ${RUN:+-run "$RUN"} ${TEST_ARGS} "${TARGET}" | tee >(go-junit-report -set-exit-code > result.xml) |
🤖 Prompt for AI Agents
In .github/workflows/integration-test.yml around lines 214 to 216, the script
uses set -x for debugging but lacks pipefail, which means failures in the
pipeline may not cause the workflow to fail. Add set -o pipefail before the go
test command to ensure the entire pipeline fails if any command errors,
improving failure detection and output reliability.
2 participants
Description
Note: AWS docs tests are still failing, looks like there is some kind of limit on the account, or tests don't clean resources properly
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit