-
Notifications
You must be signed in to change notification settings - Fork 0
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
Always run fixtures #52
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow in Changes
Poem
🪧 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
CodeRabbit Configuration File (
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.github/workflows/shared-terraform-chatops.yml (2)
Line range hint
587-591
: Fix cleanup job configurationMultiple issues in the cleanup job configuration:
- Job depends on removed
fixture_id
job- References non-existent
test_fixture_id
outputApply these fixes:
- needs: [context, terratest, fixture_id] + needs: [context, terratest, fixtures] env: MAKE_INCLUDES: Makefile AWS_REGION: us-east-2 AWS_ROLE_TO_ASSUME: ${{ needs.context.outputs.test_aws_role }} - TEST_FIXTURE_ID: ${{ needs.context.outputs.test_fixture_id }}🧰 Tools
🪛 actionlint (1.7.4)
586-586: job "cleanup" needs job "fixture_id" which does not exist in this workflow
(job-needs)
Line range hint
651-655
: Fix artifact download configurationThe artifact download step is using a removed output variable for the artifact name.
Apply this fix:
uses: actions/download-artifact@v4 with: - name: ${{ needs.context.outputs.test_fixture_id }} + name: state path: ${{ needs.context.outputs.terraform_state_path }}
🧹 Nitpick comments (1)
.github/workflows/shared-terraform-chatops.yml (1)
Line range hint
1-1
: Workflow simplification is a good improvementThe changes to always run fixtures and simplify the workflow structure are good improvements that will make the workflow more reliable and easier to maintain. The removal of conditional logic and consistent artifact handling will reduce potential points of failure.
Consider adding timeouts to the long-running jobs (fixtures, terratest, cleanup) to prevent them from running indefinitely in case of issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/shared-terraform-chatops.yml
(2 hunks)
🔇 Additional comments (1)
.github/workflows/shared-terraform-chatops.yml (1)
530-533
:
Fix job dependency on removed fixture_id
job
The terratest
job still lists fixture_id
in its needs
array, but this job has been removed. This will cause the workflow to fail.
Apply this fix:
needs: [context, pending, fixture_id]
to
needs: [context, pending, fixtures]
Likely invalid or redundant comment.
what
Summary by CodeRabbit
New Features
Bug Fixes