OCM-24672: chore: consolidate checks under make pre-push-checks#150
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR consolidates the repository's verification and CI workflows into a unified ChangesTooling consolidation and verification workflow unification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Line 21: Replace the unsafe "curl | bash" install of tflint with a pinned
release-install flow: choose a specific tflint release tag (e.g., vX.Y.Z),
download the corresponding release artifact (binary or archive) from the GitHub
releases URL for that tag, fetch the accompanying checksum/signature file for
the same tag, verify the checksum/signature before extracting/installing into
/usr/local/bin, and remove the pipe-to-shell line; reference the current tflint
install invocation (the curl -sL
https://raw.githubusercontent.com/terraform-linters/tflint/master/install_linux.sh
| bash -s -- -b /usr/local/bin line) and replace it with the pinned-download +
checksum verification steps.
In `@Makefile`:
- Around line 37-39: The verify loop in the Makefile (the for d in examples/*;
... loop) can mask failures and break on non-directory entries; change it to
iterate directories only (e.g. examples/*/), run each example inside a subshell
(cd "$d" && rm -rf .terraform .terraform.lock.hcl && terraform init && terraform
validate) and enable strict failure behavior so the first failing example stops
the target (use set -euo pipefail or ensure the subshell returns a non-zero
status to Make). Ensure you do not suppress errors with trailing && cd - that
hides failures—let the subshell propagate its exit code so Make fails fast on
terraform init/validate errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 21bcef65-dc0a-4878-bf1d-9ad81dfb9a5a
📒 Files selected for processing (9)
.github/pull_request_template.md.github/workflows/generate-terraform-docs.yaml.github/workflows/verify-min-terraform.ymlAGENTS.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdhack/run-checks.sh
💤 Files with no reviewable changes (1)
- hack/run-checks.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 12-15: The Dockerfile currently hardcodes the x86_64 AWS CLI ZIP
URL; update the RUN that downloads and installs the AWS CLI so it is
architecture-aware by selecting the proper package based on the build
architecture (e.g., check uname -m or use the Docker build ARG TARGETARCH),
choose the correct AWS CLI archive (linux-x86_64.zip vs linux-aarch64.zip),
download that URL instead of the fixed x86_64 URL, then proceed with unzip
./aws/install and cleanup as before; modify the RUN command in the Dockerfile
that contains the curl to dynamically pick the URL so arm64 builds use the
aarch64 package.
In `@README.md`:
- Around line 55-57: The README contains inconsistent Terraform version
requirements: the "make unit-tests" section says Terraform 1.6+, the
prerequisites paragraph (Terraform 1.4.6+) and the requirements table (>= 1.5.7)
disagree; update README.md so all references to Terraform version (prerequisites
paragraph, requirements table, and the "make unit-tests" line) state a single,
consistent minimum version (pick the intended minimum, e.g., 1.6.0) and ensure
the text for "make unit-tests" and the requirements table/paragraph are changed
accordingly to the chosen version string.
In `@renovate.json`:
- Line 42: The regex in the Dockerfile customManager matchString has a duplicate
named group and a bad version capture; update the pattern so group names are
unique and the version captures non-whitespace. Replace the second
"(?<depName>)" with a different name (e.g., "(?<image>)") and change
"(?<currentValue>\\s)" to "(?<currentValue>\\S+)" so the final string reads
like: "# renovate: datasource=(?<datasource>\\S+) depName=(?<depName>\\S+)(?:
versioning=(?<versioning>\\S+))?\\nFROM (?<image>\\S+):(?<currentValue>\\S+)"
ensuring the matchString in the customManager is updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 41030388-df06-4ba7-be8f-7841ccc5a8cb
📒 Files selected for processing (19)
.dockerignore.github/pull_request_template.md.github/workflows/generate-terraform-docs.yaml.github/workflows/license-check.yaml.github/workflows/verify-min-terraform.yml.gitignore.trivyignoreAGENTS.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdhack/install-release-tool.shhack/run-checks.shrenovate.jsonscripts/add-license-header.shscripts/terraform-docs.shtrivy-image.yamltrivy.yaml
💤 Files with no reviewable changes (1)
- hack/run-checks.sh
✅ Files skipped from review due to trivial changes (6)
- trivy.yaml
- .gitignore
- .dockerignore
- .trivyignore
- .github/pull_request_template.md
- AGENTS.md
amandahla
force-pushed
the
OCM-24672
branch
2 times, most recently
from
82b7d1d to
d5cfbd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/security-check-image.yaml:
- Around line 8-12: The workflow's paths filter currently lists Dockerfile,
Makefile, trivy-image.yaml and hack/install-release-tool.sh but misses files
that can change the built image or the workflow itself; update the paths block
(the 'paths:' entries) to also include .dockerignore and the workflow file
itself and consider using globbed entries like '**/Dockerfile' and
'**/.dockerignore' so changes in subdirectories are caught and the scan cannot
be bypassed by editing image-context files.
- Line 20: Add explicit persist-credentials: false to the checkout step that
uses actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd so the job does
not persist the GITHUB_TOKEN; locate the checkout step (the line with "uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd") and add the key
persist-credentials: false under that step to follow least-privilege security
best practices.
In `@hack/install-release-tool.sh`:
- Around line 39-153: Windows installs currently set dest_bin without a .exe
suffix (e.g. dest_bin="${dest_dir}/addlicense"), causing mismatches with
Makefile BIN_EXT; update each tool block (addlicense, tflint, terraform-docs,
vale, trivy) to append ".exe" for Windows targets by computing a bin suffix
(e.g. bin_ext="" and if [ "$os" = "windows" ]; then bin_ext=".exe"; fi) and then
set dest_bin="${dest_dir}/<toolname>${bin_ext}" (replace <toolname> with
addlicense, tflint, terraform-docs, vale, trivy) so installed binaries match
Makefile expectations on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 72349661-c4ad-4929-a474-0a64c23ddcc1
📒 Files selected for processing (20)
.dockerignore.github/pull_request_template.md.github/workflows/generate-terraform-docs.yaml.github/workflows/license-check.yaml.github/workflows/security-check-image.yaml.github/workflows/verify-min-terraform.yml.gitignore.trivyignoreAGENTS.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdhack/install-release-tool.shhack/run-checks.shrenovate.jsonscripts/add-license-header.shscripts/terraform-docs.shtrivy-image.yamltrivy.yaml
💤 Files with no reviewable changes (1)
- hack/run-checks.sh
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- .github/pull_request_template.md
- trivy.yaml
- .trivyignore
- AGENTS.md
- README.md
- .dockerignore
amandahla
force-pushed
the
OCM-24672
branch
2 times, most recently
from
cce7cdc to
423218d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hack/install-release-tool.sh (1)
55-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWindows install paths still miss
.exesuffix handling.
dest_binand extracted binary names are still Unix-only (addlicense,tflint, etc.). For Windows assets this can fail install/call paths (*.exe) and diverge from MakefileBIN_EXTusage.Suggested minimal fix
+bin_ext="" +if [[ "${os}" == windows* || "${os}" == mingw* || "${os}" == msys* || "${os}" == cygwin* ]]; then + bin_ext=".exe" +fi ... - dest_bin="${dest_dir}/addlicense" + dest_bin="${dest_dir}/addlicense${bin_ext}" ... - install -m 0755 "${tmp}/addlicense" "$dest_bin" + install -m 0755 "${tmp}/addlicense${bin_ext}" "$dest_bin" ... - dest_bin="${dest_dir}/tflint" + dest_bin="${dest_dir}/tflint${bin_ext}" - install -m 0755 "${tmp}/tflint" "$dest_bin" + install -m 0755 "${tmp}/tflint${bin_ext}" "$dest_bin" ... - dest_bin="${dest_dir}/terraform-docs" + dest_bin="${dest_dir}/terraform-docs${bin_ext}" - install -m 0755 "${tmp}/terraform-docs" "$dest_bin" + install -m 0755 "${tmp}/terraform-docs${bin_ext}" "$dest_bin" ... - dest_bin="${dest_dir}/vale" + dest_bin="${dest_dir}/vale${bin_ext}" - install -m 0755 "${tmp}/vale" "$dest_bin" + install -m 0755 "${tmp}/vale${bin_ext}" "$dest_bin" ... - dest_bin="${dest_dir}/trivy" + dest_bin="${dest_dir}/trivy${bin_ext}" - install -m 0755 "${tmp}/trivy" "$dest_bin" + install -m 0755 "${tmp}/trivy${bin_ext}" "$dest_bin"Also applies to: 66-67, 73-74, 80-81, 87-88, 94-95, 112-113, 123-124, 141-142, 152-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/install-release-tool.sh` around lines 55 - 56, The install script hardcodes Unix binary names (e.g., dest_bin="${dest_dir}/addlicense") and doesn't append the Windows extension; update each dest_bin and extracted-binary assignment (e.g., the occurrences for addlicense, tflint, etc.) to include the platform-aware extension by appending the existing BIN_EXT variable (or ${BIN_EXT:-} fallback) so Windows installs use *.exe; ensure all occurrences noted in the comment (lines around the repeated dest_bin assignments) are changed consistently to use the BIN_EXT-suffixed name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 9-11: The Dockerfile currently installs Terraform unpinned via the
command "microdnf install -y terraform"; update the Dockerfile to pin the
Terraform package to a specific, explicit version (for example by using a build
ARG like TERRAFORM_VERSION and installing "terraform-<version>" from the
HashiCorp repo or otherwise specifying the exact package name/version) so builds
are reproducible; ensure you update the RUN line that contains "microdnf install
-y terraform" to install the pinned package and document the chosen
TERRAFORM_VERSION ARG/default.
In `@renovate.json`:
- Around line 73-91: The Renovate package rule "Terraform CLI (CI)" currently
uses matchManagers: "custom.regex" and matchFileNames: ".github/workflows/**"
but there is no corresponding custom regex manager defined, so the rule never
matches; add a custom regex manager entry under customManagers (a manager key
that matches the "custom.regex" identifier) that targets ".github/workflows/**"
and provides a regex to find/update the terraform_version value (e.g., capture
terraform_version: "1.x.x") so the existing rule (groupName "Terraform CLI
(CI)", matchPackageNames "hashicorp/terraform") can detect and update the
hardcoded version in workflow files.
---
Duplicate comments:
In `@hack/install-release-tool.sh`:
- Around line 55-56: The install script hardcodes Unix binary names (e.g.,
dest_bin="${dest_dir}/addlicense") and doesn't append the Windows extension;
update each dest_bin and extracted-binary assignment (e.g., the occurrences for
addlicense, tflint, etc.) to include the platform-aware extension by appending
the existing BIN_EXT variable (or ${BIN_EXT:-} fallback) so Windows installs use
*.exe; ensure all occurrences noted in the comment (lines around the repeated
dest_bin assignments) are changed consistently to use the BIN_EXT-suffixed name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7d915e88-0758-49ec-a29d-4e18d784dd34
📒 Files selected for processing (20)
.dockerignore.github/pull_request_template.md.github/workflows/generate-terraform-docs.yaml.github/workflows/license-check.yaml.github/workflows/security-check-image.yaml.github/workflows/verify-min-terraform.yml.gitignore.trivyignoreAGENTS.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdhack/install-release-tool.shhack/run-checks.shrenovate.jsonscripts/add-license-header.shscripts/terraform-docs.shtrivy-image.yamltrivy.yaml
💤 Files with no reviewable changes (2)
- hack/run-checks.sh
- .github/workflows/license-check.yaml
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- .trivyignore
- .github/pull_request_template.md
- .dockerignore
- trivy.yaml
- README.md
amandahla
force-pushed
the
OCM-24672
branch
5 times, most recently
from
76a3790 to
a2fff9a
Compare
amandahla
changed the title
|
@amandahla: This pull request references OCM-24672 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
This condition calls length() before the null guard, so the default null value can fail with "argument must not be null". I think we could maybe check for null/private first, or use a ternary before touching length().
in the main.tf file
condition = ((length(var.aws_additional_control_plane_security_group_ids) >= 0 && var.private && tonumber(format("%03d%03d%03d", split(".", var.openshift_version)...)) >= 4017002) == true || var.aws_additional_control_plane_security_group_ids == null || var.private == false) == true
amandahla
force-pushed
the
OCM-24672
branch
4 times, most recently
from
108ea4d to
0a834c0
Compare
|
/test ci/prow/verify-format |
|
/test verify-format |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
variables.tf (1)
254-260: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the OpenShift minimum for this feature in the variable docs.
Line 257 should explicitly state this input requires OpenShift 4.17.2+ (for private clusters), and the README should mirror the same minimum requirement.
💡 Suggested update
variable "aws_additional_control_plane_security_group_ids" { type = list(string) default = null - description = "The additional security group IDs to be added to the control plane VPC endpoint." + description = "The additional security group IDs to be added to the control plane VPC endpoint. For private clusters, this requires OpenShift version 4.17.2 or later." validation { condition = var.aws_additional_control_plane_security_group_ids == null ? true : length(var.aws_additional_control_plane_security_group_ids) > 0 error_message = "Security group list cannot be empty." } }As per coding guidelines:
**/variables.tf: "Document minimum OpenShift version requirements in the variable description and README when a feature needs a specific minimum version."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@variables.tf` around lines 254 - 260, Update the variable description for aws_additional_control_plane_security_group_ids to explicitly state the OpenShift minimum requirement (e.g., "Requires OpenShift 4.17.2+ for private clusters") and mirror that same minimum-version note in the module README; ensure the wording appears in the variable's description block and the README's feature/requirements section so users see the 4.17.2+ requirement clearly.
♻️ Duplicate comments (1)
hack/install-release-tool.sh (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWindows installs still won't satisfy the Makefile targets.
os="$(uname ...)"never normalizes Git Bash/MSYS/Cygwin towindows, and these branches still install non-.exepaths from ZIP payloads. On Windows,make toolsexpectsbin/<tool>.exe, so addlicense/tflint/terraform-docs/vale/checkov remain broken even though gitleaks already handles this correctly.Suggested direction
+bin_ext="" +case "$os" in + mingw*|msys*|cygwin*|windows*) + os="windows" + bin_ext=".exe" + ;; +esac ... - dest_bin="${dest_dir}/addlicense" + dest_bin="${dest_dir}/addlicense${bin_ext}"Apply the same
bin_exthandling to the other Windows ZIP branches and install the extracted*.exepayloads whenarchive_ext="zip".Also applies to: 57-68, 75-82, 89-96, 114-125, 143-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/install-release-tool.sh` around lines 17 - 19, The script currently sets arch and os but doesn't normalize Git Bash/MSYS/Cygwin to "windows" and doesn't set bin_ext/handle ZIP branches for tools other than gitleaks; update the os detection so uname variants used by Git Bash/MSYS/Cygwin map to "windows" (so os becomes "windows"), set archive_ext="zip" and bin_ext=".exe" when os is "windows", and in each ZIP-handling branch for addlicense, tflint, terraform-docs, vale, checkov (the blocks that currently extract zip archives) ensure you install the extracted *.exe payloads into bin/<tool>.exe (using the bin_ext variable) just like the gitleaks branch does; keep using the existing variables arch, os, archive_ext, and bin_ext to locate and name the installed files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 150-155: The skip case in the Makefile loop doesn't match because
d includes a trailing slash (e.g., modules/rosa-cluster-hcp/), so the pattern
case "$$d" in */rosa-cluster-hcp) never triggers; update the case to match the
trailing slash (e.g., change the pattern to */rosa-cluster-hcp/) or normalize
the value (e.g., use "${d%/}" in the case) so the rosa-cluster-hcp directory is
properly skipped in the for d in modules/*/; do ... CHECKOV invocation.
---
Outside diff comments:
In `@variables.tf`:
- Around line 254-260: Update the variable description for
aws_additional_control_plane_security_group_ids to explicitly state the
OpenShift minimum requirement (e.g., "Requires OpenShift 4.17.2+ for private
clusters") and mirror that same minimum-version note in the module README;
ensure the wording appears in the variable's description block and the README's
feature/requirements section so users see the 4.17.2+ requirement clearly.
---
Duplicate comments:
In `@hack/install-release-tool.sh`:
- Around line 17-19: The script currently sets arch and os but doesn't normalize
Git Bash/MSYS/Cygwin to "windows" and doesn't set bin_ext/handle ZIP branches
for tools other than gitleaks; update the os detection so uname variants used by
Git Bash/MSYS/Cygwin map to "windows" (so os becomes "windows"), set
archive_ext="zip" and bin_ext=".exe" when os is "windows", and in each
ZIP-handling branch for addlicense, tflint, terraform-docs, vale, checkov (the
blocks that currently extract zip archives) ensure you install the extracted
*.exe payloads into bin/<tool>.exe (using the bin_ext variable) just like the
gitleaks branch does; keep using the existing variables arch, os, archive_ext,
and bin_ext to locate and name the installed files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: de14cd3f-1aab-459b-9192-f8aac7b7fbb2
📒 Files selected for processing (35)
.coderabbit.yaml.dockerignore.github/pull_request_template.md.github/workflows/check-commit-format.yml.github/workflows/generate-terraform-docs.yaml.github/workflows/license-check.yaml.github/workflows/test.yml.github/workflows/verify-min-terraform.yml.gitignore.gitleaks.toml.trivyignoreAGENTS.mdCONTRIBUTING.mdDockerfileMakefileREADME.mdcheckov.yamlexamples/rosa-hcp-private-shared-vpc/outputs.tfexamples/rosa-hcp-private-with-additional-control-plane-security-groups/outputs.tfexamples/rosa-hcp-private/outputs.tfexamples/rosa-hcp-public-unmanaged-oidc/outputs.tfexamples/rosa-hcp-public-with-multiple-machinepools-and-idps/outputs.tfexamples/rosa-hcp-public/outputs.tfhack/aws-cli-public-key.aschack/checksums/checkov-3.2.529.sha256sumshack/install-release-tool.shhack/run-checks.shmain.tfmodules/bastion-host/main.tfmodules/oidc-config-and-provider/main.tfrenovate.jsonscripts/add-license-header.shscripts/terraform-docs.shtrivy.yamlvariables.tf
💤 Files with no reviewable changes (7)
- hack/run-checks.sh
- trivy.yaml
- modules/oidc-config-and-provider/main.tf
- .github/workflows/test.yml
- .github/workflows/license-check.yaml
- .trivyignore
- modules/bastion-host/main.tf
✅ Files skipped from review due to trivial changes (8)
- .coderabbit.yaml
- hack/checksums/checkov-3.2.529.sha256sums
- .github/pull_request_template.md
- .gitleaks.toml
- examples/rosa-hcp-private-with-additional-control-plane-security-groups/outputs.tf
- .dockerignore
- README.md
- .gitignore
Unify the module merge gate in Makefile (inline pre-push-checks), add make tools for pinned CLI installs, extend the CI client image, add verify-min-terraform workflow, remove test.yml, and align docs. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Amanda Hager Lopes de Andrade Katz <amanda.katz@redhat.com>
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, olucasfreitas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/rosa-hcp-private Failures not related to the PR: error while cleaning VPCs |
|
@amandahla: Overrode contexts on behalf of amandahla: ci/prow/rosa-hcp-private, ci/prow/rosa-hcp-public DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
Bot
merged commit 802ee75
into
terraform-redhat:main
3 participants
PR Summary
Unifies module merge gate checks under
make pre-push-checkswith pinned CLI tool versions, addshack/install-release-tool.shfor release binary installs with SHA256 verification, replaces Trivy with Checkov + Gitleaks, hardens the Prow CI Dockerfile (ubi-minimal, GPG-verifiedAWS CLI, SHA256-verified ROSA), and adds GitHub Actions
verify-min-terraform.yml.Detailed Description of the Issue
Before this change, the module's local verification flow was fragmented across multiple Makefile targets (
verify,verify-gen,lint,unit-tests,license-check,docs-lint), and the Prow CI image lacked supply-chain hardening (no signature verification for CLI downloads,used full UBI instead of ubi-minimal). Security scanning relied on Trivy alone. Tool versions (terraform-docs, vale) were either hardcoded in scripts or required manual installs, making reproducible environments difficult.
This consolidation aligns the local developer flow with the planned single Prow presubmit, reduces attack surface in the CI client image, and brings security tooling in line with current best practices (Gitleaks for secrets, Checkov for Terraform static analysis).
Related Issues and PRs
Type of Change
Previous Behavior
make verify,make verify-gen,make lint,make unit-tests,make license-check, andmake docs-lintindividually or via the shell scripthack/run-checks.sh basic(invoked asmake basic-checks).ubi9/ubi:latestand installed AWS CLI, ROSA CLI, and Terraform without cryptographic verification.license-check.yaml) and tests (test.yml).make verifyandmake verify-genindividually.Behavior After This Change
make pre-push-checksonce (equivalent to the planned single Prow presubmit); individual targets remain available for debugging.make toolsinstalls pinned CLI versions (addlicense, terraform-docs, tflint, vale, checkov, gitleaks) from GitHub releases intobin/usinghack/install-release-tool.sh, which verifies downloads with SHA256 checksums (or GPG for AWS CLI when building the Dockerfile).ubi9/ubi-minimal:9.8(pinned minor), verifies AWS CLI with GPG againsthack/aws-cli-public-key.asc, verifies ROSA CLI with SHA256 from upstreamsha256sum.txt, and installs Terraform via YUM repo at pinnedTERRAFORM_VERSION.make security-check(separate from merge gate) runs Gitleaks (secret scan) and Checkov (Terraform static analysis); Trivy is removed.verify-min-terraform.ymlvalidates examples at the minimum Terraform version from rootversions.tf;license-check.yamlandtest.ymlare consolidated into the planned Prow presubmit flow (Prow config pending inopenshift/release).make pre-push-checkswith inline breakdown of steps..coderabbit.yamldisables Trivy (replaced by Checkov)..gitleaks.tomlallowlistsbin/,.terraform/, and.terraform-docs-cache/;checkov.yamlskipsmodules/rosa-cluster-hcp/main.tf(lifecycle precondition parse limitation).How to Test (Step-by-Step)
Preconditions
versions.tfminimum)bin/directory (or deletebin/to force fresh install)Test Steps
make pre-push-checksfrom the repo roottoolsstep installing pinned CLIs intobin/verify,verify-gen,lint,unit-tests,license-check,docs-lintmake security-checkto test Gitleaks + Checkovdocker build -t test-client .bin/terraform-docs version,bin/vale --version,bin/addlicense --version,bin/tflint --version,bin/checkov --version,bin/gitleaks versionExpected Results
make pre-push-checkscompletes with exit 0 (all checks pass)bin/directory containsaddlicense,terraform-docs,tflint,valeat Makefile-pinned versionsmake security-checkruns Gitleaks and Checkov with no CRITICAL or HIGH findings (exit 0)terraform version,aws --version,rosa versionoutput expected pinned versionsProof of the Fix
make pre-push-checkslocally (all steps passed)Breaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
rhcsresources/variables), I linked official Red Hat or cited ROSA HCP documentation that supports reference alignment, or I explained why the change still belongs in-repo perModule scope (AWS-only vs core HCP)in.cursor/rules/rosa-hcp-terraform.mdc.[JIRA-TICKET] | [TYPE]: <MESSAGE>.make pre-push-checkspasses (or each step:verify,verify-gen,lint,unit-tests,license-check,docs-lint).make terraform-docs).Summary by CodeRabbit
New Features
Bug Fixes
Documentation