Terraform Static Check - PILOT update #72
Conversation
tmikula-dev
requested review from
ABMC831,
Zejnilovic,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
WalkthroughThe PR introduces detect-and-execute CI workflows that conditionally run checks when relevant files change (terraform, Dockerfile, Python), adds full-repository Trivy scans, renames/updates workflow metadata and action versions, and removes the legacy multi-job Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Event (PR / Push)
participant Detect as Detect Job
participant Cond as Condition (output *_changed)
participant Check as Analysis Job (TFLint / Trivy / Py checks)
participant Noop as Noop Job
participant Upload as SARIF / Results Upload
GH->>Detect: trigger workflow
Detect->>Detect: compute git diff range (PR: base..head, Push: prev..current)
Detect->>Detect: inspect files (terraform/*.tf, Dockerfile, *.py)
Detect->>Cond: export *_changed (true/false)
alt *_changed == "true"
Cond->>Check: run relevant analysis job(s)
Check->>Upload: produce & upload SARIF/results
else *_changed == "false"
Cond->>Noop: run noop (log no changes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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/workflows/check_terraform.yml (1)
19-19: Minor: Clarify the comment explaining workflow intent.The comment "This job is implemented this way, because we need workflow mandatory" is unclear. Consider rephrasing to explain that the detect job ensures the workflow always executes (even with no changes) to maintain mandatory status in branch protection rules.
- ## This job is implemented this way, because we need workflow mandatory + ## Detect job ensures workflow runs regardless of changes, making it mandatory in branch protection rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/check_terraform.yml(3 hunks)
🔇 Additional comments (4)
.github/workflows/check_terraform.yml (4)
45-46: Conditional execution structure looks good.The
needs: detectdependency andif: needs.detect.outputs.changed == 'true'conditions correctly gate the trivy and tflint jobs, ensuring they skip when terraform/ has no changes while maintaining overall workflow status via the noop job.Also applies to: 83-84
122-127: Noop job effectively preserves mandatory workflow status.The noop job ensures at least one job always completes successfully, preventing the workflow from being skipped entirely when terraform/ hasn't changed—a critical design pattern for making this workflow mandatory in branch protection.
25-25: Consistent checkout action upgrade to v5.All three jobs (detect, trivy, tflint) uniformly upgrade to
actions/checkout@v5while preserving thefetch-depth: 0configuration needed for git history operations.Also applies to: 49-49, 87-87
32-42: I'll verify the critical claim about the git diff edge case and GitHub Actions behavior.Let me verify one more critical detail about how
HEAD~1...HEADbehaves on the first commit.Critical issue is valid, but one suggested fix is flawed. The
HEAD~1...HEADapproach does not gracefully handle first commits—it also fails with a fatal error.Using
git diff HEAD^ HEADon the first commit results in "fatal: ambiguous argument 'HEAD^': unknown revision or path not in the working tree". The same applies toHEAD~1...HEAD. The review's claim that this approach "handles the initial commit gracefully" is incorrect.The conditional check approach (using
git rev-parse "${{ github.sha }}~1"to verify the parent exists) remains the correct solution. Remove the misleading suggestion aboutHEAD~1...HEAD.Likely an incorrect or invalid review comment.
tmikula-dev
added
the
work in progress
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (3)
21-26: Critical: Docker build fails due to missing COPY source paths.The pipeline failures show COPY commands referencing build args that don't exist by default:
./trusted_certsand./sasl_ssl_artifacts. When these directories are not provided at build time, the COPY operations fail, breaking the Docker image build.Either provide default directories that exist in the repository, make these COPY operations conditional with fallbacks, or document that these build args are required and must be supplied during builds.
Suggested fix: Make COPY operations conditional or require the build args:
# Option 1: Make paths optional by checking if they exist ARG TRUSTED_SSL_CERTS=./trusted_certs ARG SASL_SSL_ARTIFACTS=./sasl_ssl_artifacts # Create empty directories as fallbacks RUN mkdir -p /opt/certs/ /opt/sasl_ssl_artifacts/ # Conditionally copy only if directories exist and have files COPY --chown=0:0 ${TRUSTED_SSL_CERTS}/*.pem /opt/certs/ 2>/dev/null || true COPY ${SASL_SSL_ARTIFACTS} /opt/sasl_ssl_artifacts/ 2>/dev/null || trueOr:
# Option 2: Require the build args and ensure they're provided ARG TRUSTED_SSL_CERTS ARG SASL_SSL_ARTIFACTS COPY ${TRUSTED_SSL_CERTS} /opt/certs/ COPY ${SASL_SSL_ARTIFACTS} /opt/sasl_ssl_artifacts/
18-18: Minor: FROM --platform flag uses constant value.Hadolint warns against using constant platform flags in FROM statements, as this limits portability and should typically be left to runtime configuration or build context.
This is a low-priority warning but can be addressed by removing the platform specification if cross-platform builds aren't a requirement, or documenting the ARM64-specific constraint.
66-66: Verify LDFLAGS path for librdkafka.Line 66 sets
LDFLAGS="-L/opt", but line 53 installs librdkafka to the default location (likely/usr/local/lib). The LDFLAGS path should match the installation directory:- CPPFLAGS="-I/usr/local/include" LDFLAGS="-L/opt" python setup.py install + CPPFLAGS="-I/usr/local/include" LDFLAGS="-L/usr/local/lib" python setup.py install
🧹 Nitpick comments (3)
Dockerfile (1)
50-53: Consider adding checksums for downloaded binaries.Lines 50 and 63 download source archives without verifying integrity. Adding checksums improves security and reproducibility:
RUN \ mkdir -p /tmp/env-install-workdir/librdkafka && \ cd /tmp/env-install-workdir/librdkafka && \ wget https://github.com/edenhill/librdkafka/archive/v2.4.0.tar.gz && \ echo "expected_sha256_hash v2.4.0.tar.gz" | sha256sum -c - && \ tar -xf v2.4.0.tar.gz && \ # ... rest of buildAlso applies to: 63-66
.github/workflows/check_python.yml (1)
35-44: Change detection logic is sound but consider edge cases.The detect job correctly uses git diff with proper branching logic for PRs vs pushes. However, the pattern
^(src|tests)/matches files at any path depth; ensure this covers nested directories likesrc/module/file.py(which it does).One edge case: If only configuration files like
setup.cfgorpytest.inichange, Python analysis still runs. Consider whether config-only changes should trigger the workflow..github/workflows/check_docker.yml (1)
40-40: Dockerfile detection pattern matches only root-level Dockerfile.Line 40 uses pattern
^Dockerfile$, which only matches the Dockerfile at the repository root. If other Dockerfiles exist in subdirectories (e.g.,services/api/Dockerfile), they won't trigger this workflow.If the repo has multiple Dockerfiles, update the pattern:
if git diff --name-only "$RANGE" | grep -qE '(^|/)Dockerfile'; thenOtherwise, document that only the root Dockerfile is scanned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/check_docker.yml(1 hunks).github/workflows/check_python.yml(1 hunks).github/workflows/check_terraform.yml(2 hunks).github/workflows/test.yml(0 hunks)DEVELOPER.md(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test.yml
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- DEVELOPER.md
🧰 Additional context used
🪛 GitHub Actions: Static Docker Check
Dockerfile
[error] 5-5: failed to calculate checksum of ref ac1187db-1850-4397-a0f7-3b6e6ef293d6::/trusted_certs: "/trusted_certs" not found
[error] 7-7: failed to calculate checksum of ref ac1187db-1850-4397-a0f7-3b6e6ef293d6::/sasl_ssl_artifacts: not found
[warning] 18-18: FromPlatformFlagConstDisallowed: FROM --platform flag should not use constant value "linux/arm64"
[error] 74-74: COPY $SASL_SSL_ARTIFACTS /opt/sasl_ssl_artifacts/ exited with error due to missing source path
🔇 Additional comments (5)
.github/workflows/check_python.yml (2)
127-128: Pytest coverage threshold at 80% is strict—verify it's achievable.The
--cov-fail-under=80flag enforces 80% coverage on tests. Ensure the test suite actually meets this threshold; if tests fail due to coverage gaps, CI will block merges.Consider running locally to verify coverage:
pytest --cov=. -v tests/ --cov-fail-under=80
103-105: This review comment is incorrect and should be dismissed.The workflow's approach is intentional and documented. The
detectjob gates all downstream analysis jobs to run only when Python files change (lines 48-49:if: needs.detect.outputs.python_changed == 'true'). When Python changes are detected, validating the full codebase—not just diffs—is the documented, recommended practice per DEVELOPER.md, which explicitly instructs developers to run checks on "all files currently tracked by Git" usingblack $(git ls-files '*.py')andpylint $(git ls-files '*.py').This two-tier design (detect → validate full consistency) is sound: it avoids redundant checks when no Python changes exist while ensuring codebase-wide quality standards when changes occur.
Likely an incorrect or invalid review comment.
.github/workflows/check_terraform.yml (3)
35-44: Terraform change detection aligns with Python and Docker workflows.The detect job uses identical logic and structure to the Python and Docker workflows, which is excellent for consistency. The pattern
^terraform/correctly identifies changes in the terraform directory.
91-97: Trivy scan from terraform/ subdirectory is appropriate.The Trivy scan at line 94 runs from the terraform/ working directory, scanning the current directory (
.). This is correct and aligns with the Python workflow's approach of scanning only the relevant code paths.
48-49: I need to gather more specific information about TFLint's version history and when SARIF support was introduced.TFLint SARIF support is well-established; no action needed.
SARIF format support was added to TFLint in v0.35.0, and the CLI currently supports
-f sarifas a documented output format option. When usingterraform-linters/setup-tflintwithtflint_version: latest, the action retrieves the latest TFLint version, which includes stable SARIF support. The concern about version-dependent SARIF availability is not applicable—TFLint's 0.x version scheme does not have a v1.40, and SARIF has been a standard, supported output format for years without breaking changes.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
Please look at the suggested solution. This PILOT has a team goal to understand and set the standard of working with workflows and scanners inside our projects. I did follow the splitting the workflows for determined purposes. Open to the discussion. |
.github/workflows/check_python.yml
Outdated
| id: check-types | ||
| run: mypy . | ||
|
|
||
| trivy-python: |
There was a problem hiding this comment.
I believe that in PR Trivy should should be limited to all chages files.
I believe we should introduce Trivy run once a "day"|"???" across entrire workspace.
There was a problem hiding this comment.
See the new version in commit: 0e6e234. And share your thoughts:)
| with: | ||
| sarif_file: ${{ github.workspace }}/tflint_terraform.sarif | ||
|
|
||
| trivy-terraform: |
There was a problem hiding this comment.
Here I do not agree with solution. I would isolate Trivy to its own PR changes related flow.
There was a problem hiding this comment.
It is now isolated in the automatic midnight test scan. Only place, where the Trivy is still implemented is in docker-check. This decision was due to your recommendation. You can see the implementation in: 0e6e234.
src/conf_path.py
Outdated
| import os | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
This and others are fake changes for testing?
There was a problem hiding this comment.
Yes, these changes are made, so I can show, how workflow behaves here on GH. Will not be part of final solution.
DEVELOPER.md
Outdated
| ### Run Trivy | ||
|
|
||
| For running Trivy tool locally run the following command from the root file: | ||
| For running Trivy tool locally run you can run one of following commands from the root file: |
There was a problem hiding this comment.
| For running Trivy tool locally run you can run one of following commands from the root file: | |
| For running Trivy tool locally you can run one of following commands from the root file: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/check_terraform.yml (1)
1-82: Note: This PR mixes multiple concerns (Trivy isolation, Terraform refactoring, documentation updates).The previous reviewer (miroslavpojer) suggested isolating Trivy changes to a separate PR. However, this PR combines:
- Addition of
trivy_auto_repository_scan.yml(new full-repository Trivy scan)- Removal of Trivy from
check_terraform.yml(shift to git-diff detection)- Documentation updates to DEVELOPER.md (conflicting scope descriptions)
While each change has merit, mixing them makes the PR harder to review, test, and potentially rollback if issues arise. Consider whether this grouping aligns with the "pilot" goals mentioned in the PR objectives (understanding and setting standards for workflows).
Would you like to discuss splitting this work into focused PRs (e.g., Trivy-specific PR, Terraform refactoring PR, documentation PR)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/check_docker.yml(1 hunks).github/workflows/check_pr_release_notes.yml(2 hunks).github/workflows/check_python.yml(1 hunks).github/workflows/check_terraform.yml(2 hunks).github/workflows/trivy_auto_repository_scan.yml(1 hunks)DEVELOPER.md(1 hunks)src/conf_path.py(0 hunks)terraform/lambda.tf(1 hunks)
💤 Files with no reviewable changes (1)
- src/conf_path.py
✅ Files skipped from review due to trivial changes (1)
- terraform/lambda.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/check_docker.yml
- .github/workflows/check_python.yml
🔇 Additional comments (6)
.github/workflows/check_pr_release_notes.yml (1)
1-1: Naming consistency improvements.The workflow and job names have been updated to match and are now more concise. This improves consistency with other workflows in the suite and enhances readability without affecting functionality.
Also applies to: 10-10
.github/workflows/check_terraform.yml (3)
76-82: Noop job appropriately preserves workflow status.The new
noopjob ensures the workflow has a passing status when no Terraform changes are detected, avoiding false failures in branch protection rules.
19-44: Git diff detection logic verified; action versions current and acceptable.The
actions/checkout@v5action uses the latest stable version released August 11, 2025. Thesetup-tflint@v6action is the official setup action for TFLint. The git diff logic for both PR and push contexts is working correctly as confirmed by the script execution—terraform changes are properly detected in both scenarios.No changes needed to the code shown at lines 19-44. The shell-based change detection successfully replaced Trivy-based logic with a cleaner, more efficient approach.
46-74: All action versions are current and appropriate.actions/checkout@v5 is the latest release. terraform-linters/setup-tflint@v6 tracks the v6 line, and github/codeql-action/upload-sarif@v4 receives the latest v4 releases. Using major version pins enables automatic receipt of security updates and bug fixes within each major version line—a best practice for GitHub Actions workflows.
.github/workflows/trivy_auto_repository_scan.yml (1)
1-36: Workflow configuration is correct and up-to-date.The new
trivy_auto_repository_scan.ymlworkflow appropriately separates full-repository Trivy scanning from refactored Terraform-specific checks. The version aquasecurity/[email protected] is the latest release as of Aug 29, 2025, confirming the action reference is current. Permissions, checkout, Trivy setup, and SARIF upload are correctly configured.DEVELOPER.md (1)
131-144: Clarify documentation scope: "scan changes" vs full filesystem scan.Line 131 states Trivy scans "changes," but the provided
trivy fscommands (lines 140–141) scan entire directory structures (.orterraform/), not Git-tracked changes. Update the documentation to clarify that local scans perform full-filesystem scanning.Note: Trivy is not currently configured in CI workflows, so there is no local-to-CI behavior mismatch to align.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
DEVELOPER.md (1)
131-145: ---Documentation and Trivy commands are accurate.
The command syntax is correct and aligns with current Trivy CLI conventions. The shift from IaC-specific scanning to project-wide scanning is well-reflected in the documentation.
Optional: Consider adding a brief note distinguishing the scan types—
trivy fsfor comprehensive filesystem scanning andtrivy configspecifically for configuration files—to clarify when each command is appropriate.
2 participants
Release Notes:
Summary by CodeRabbit
Chores
Documentation