HYPERFLEET-525 - ci: add CI validation targets and OWNERS for pre submit#19
HYPERFLEET-525 - ci: add CI validation targets and OWNERS for pre submit#19tzhou5 wants to merge 4 commits intoopenshift-hyperfleet:mainfrom
Conversation
WalkthroughAdds two Makefile variables, Sequence Diagram(s)sequenceDiagram
participant Dev as "Developer / CI (Makefile)"
participant Helm as "Helm CLI"
participant K8s as "Kubernetes API"
Dev->>Helm: run `helm upgrade --install` (preface with DRY_RUN_FLAG if set)
Helm->>K8s: render templates / validate (or dry-run)
K8s-->>Helm: validation/result
Helm-->>Dev: exit status and output
sequenceDiagram
participant Dev as "Developer / CI (Makefile)"
participant TF as "Terraform CLI"
participant Cloud as "Cloud Provider API"
Dev->>TF: run `terraform apply` (include AUTO_APPROVE_FLAG if set)
TF->>Cloud: execute apply (or plan/apply sequence)
Cloud-->>TF: apply result
TF-->>Dev: exit status and output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![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 (2)
Makefile (2)
268-274: Consider making shellcheck a hard dependency for CI.The current implementation silently skips shellcheck validation when the tool isn't installed. This is convenient for local development but could mask shell script issues in CI environments.
Consider either:
- Making shellcheck installation a prerequisite in your CI environment
- Failing the target in CI contexts (e.g., check for
CIenvironment variable)♻️ Optional: Fail in CI when shellcheck is missing
.PHONY: lint-shellcheck lint-shellcheck: ## Validate shell scripts with shellcheck `@if` command -v shellcheck >/dev/null 2>&1; then \ find . -name '*.sh' -not -path './.terraform/*' -not -path './.git/*' -exec shellcheck {} +; \ + elif [ -n "$$CI" ]; then \ + echo "ERROR: shellcheck is required in CI but not installed"; exit 1; \ else \ echo "WARN: shellcheck not installed, skipping"; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 268 - 274, The lint-shellcheck Makefile target currently skips validation when shellcheck is missing; update it so CI does not silently pass: in the lint-shellcheck recipe (the target named lint-shellcheck that runs the find ... -exec shellcheck {} + command) change the missing-tool branch to fail in CI by checking the CI environment variable (e.g., if [ -n "$CI" ] then echo error and exit 1), or make shellcheck a hard prerequisite by always exiting non-zero when command -v shellcheck fails; ensure the change references the existing shellcheck check and the lint-shellcheck target so CI will fail instead of printing a warn message.
354-357: Inconsistent use of-auto-approvevs$(AUTO_APPROVE).
destroy-terraformuses a hardcoded-auto-approvewhileinstall-terraform(line 227) uses$(AUTO_APPROVE). This means destroy is always non-interactive, even when the user might want confirmation.If this is intentional for CI cleanup safety, consider adding a comment explaining the design decision. Otherwise, consider using
$(AUTO_APPROVE)for consistency.♻️ Optional: Use $(AUTO_APPROVE) for consistency
.PHONY: destroy-terraform destroy-terraform: check-terraform check-tf-files ## Destroy Terraform-managed infrastructure cd $(TF_DIR) && terraform init -backend-config=$(TF_BACKEND) - cd $(TF_DIR) && terraform destroy -var-file=$(TF_VARS) -auto-approve + cd $(TF_DIR) && terraform destroy -var-file=$(TF_VARS) $(AUTO_APPROVE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 354 - 357, The destroy-terraform target currently hardcodes -auto-approve while install-terraform uses $(AUTO_APPROVE); update the destroy-terraform recipe (target destroy-terraform) to use $(AUTO_APPROVE) instead of the literal -auto-approve for consistency with install-terraform, or if non-interactive destroy is intentional, add a clarifying comment above the destroy-terraform target explaining the safety/CI rationale; reference the make variable AUTO_APPROVE and the target name destroy-terraform when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OWNERS`:
- Around line 1-35: Replace the numeric GitHub ID "86254860" in the OWNERS file
(present under both approvers and reviewers) with the corresponding GitHub
username or an alias defined in OWNERS_ALIASES; update both occurrences so Prow
recognizes the account and keep the rest of the approvers/reviewers entries
unchanged.
---
Nitpick comments:
In `@Makefile`:
- Around line 268-274: The lint-shellcheck Makefile target currently skips
validation when shellcheck is missing; update it so CI does not silently pass:
in the lint-shellcheck recipe (the target named lint-shellcheck that runs the
find ... -exec shellcheck {} + command) change the missing-tool branch to fail
in CI by checking the CI environment variable (e.g., if [ -n "$CI" ] then echo
error and exit 1), or make shellcheck a hard prerequisite by always exiting
non-zero when command -v shellcheck fails; ensure the change references the
existing shellcheck check and the lint-shellcheck target so CI will fail instead
of printing a warn message.
- Around line 354-357: The destroy-terraform target currently hardcodes
-auto-approve while install-terraform uses $(AUTO_APPROVE); update the
destroy-terraform recipe (target destroy-terraform) to use $(AUTO_APPROVE)
instead of the literal -auto-approve for consistency with install-terraform, or
if non-interactive destroy is intentional, add a clarifying comment above the
destroy-terraform target explaining the safety/CI rationale; reference the make
variable AUTO_APPROVE and the target name destroy-terraform when making the
change.
| approvers: | ||
| - "86254860" | ||
| - aredenba-rh | ||
| - ciaranRoche | ||
| - crizzo71 | ||
| - jsell-rh | ||
| - mbrudnoy | ||
| - Mischulee | ||
| - pnguyen44 | ||
| - rafabene | ||
| - rh-amarin | ||
| - tirthct | ||
| - tzhou5 | ||
| - vkareh | ||
| - xueli181114 | ||
| - yasun1 | ||
| - yingzhanredhat | ||
|
|
||
| reviewers: | ||
| - "86254860" | ||
| - aredenba-rh | ||
| - ciaranRoche | ||
| - crizzo71 | ||
| - jsell-rh | ||
| - mbrudnoy | ||
| - Mischulee | ||
| - pnguyen44 | ||
| - rafabene | ||
| - rh-amarin | ||
| - tirthct | ||
| - tzhou5 | ||
| - vkareh | ||
| - xueli181114 | ||
| - yasun1 | ||
| - yingzhanredhat |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the numeric ID corresponds to a valid GitHub user
# Check if the numeric ID resolves to a GitHub user
gh api users/86254860 --jq '.login' 2>/dev/null || echo "Could not resolve numeric ID to username"Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 233
🏁 Script executed:
# Search for OWNERS file documentation and CI configuration
fd -type f -name "*.md" -o -name "*.yaml" -o -name "*.yml" | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 251
🏁 Script executed:
# Check if "86254860" appears elsewhere in the repository with context
rg "86254860" -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 251
🏁 Script executed:
# Look for Prow or CI configuration files
fd -type f \( -name ".prow.yaml" -o -name "prow*" -o -name "*prow*" -o -name "OWNERS" \) | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 251
🏁 Script executed:
# Check the git history of the OWNERS file to see if there's context about the numeric ID
git log --oneline OWNERS 2>/dev/null | head -10Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 161
🏁 Script executed:
# Look at the specific commit that added the OWNERS file for context
git show 0c924fc --statRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 3495
🏁 Script executed:
# Search for any README or documentation about CI/OWNERS setup
find . -name "README*" -o -name "*.md" | xargs grep -l "OWNERS\|CI\|Prow" 2>/dev/null | head -5Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 131
🏁 Script executed:
# Check if there are any GitHub workflow files or references to Prow
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "prow\|OWNERS" 2>/dev/null | head -10Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 63
🏁 Script executed:
# Check the README.md for CI/OWNERS documentation
head -100 README.mdRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 3405
🏁 Script executed:
# Check the Makefile for CI targets and OWNERS usage
grep -i "owner\|prow\|ci" Makefile | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 627
🏁 Script executed:
# Look for any configuration that might explain the numeric ID format
find . -name ".prow.yaml" -o -name "prow.yaml" -o -name "*prow*" 2>/dev/nullRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 157
🏁 Script executed:
# Check the Prow terraform configuration files
cat terraform/envs/gke/dev-prow.tfvarsRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2766
🏁 Script executed:
# Look for any .prow.yaml or prow configuration in the repo
find . -type f -name "*prow*" | xargs cat 2>/dev/nullRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 3133
🏁 Script executed:
# Check if there's documentation about the OWNERS file format in git history or comments
git log --all --grep="OWNERS\|86254860" --onelineRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 279
🏁 Script executed:
# Search for GitHub documentation or OpenShift docs references about OWNERS file format
rg "OWNERS.*format|numeric.*id|user.*id" -iRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 63
🏁 Script executed:
# Check if "86254860" is a real GitHub user by looking at the PR history
git log --all --oneline | grep "86254860"Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 198
🏁 Script executed:
# Search for Prow configuration or OWNERS file documentation
rg "Prow\|OWNERS" -i README.md | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 63
🏁 Script executed:
# Check if there are any comments or documentation in the OWNERS file itself
cat OWNERSRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 481
🏁 Script executed:
# Look for any Prow-specific configuration or plugin files
find . -name "plugins.yaml" -o -name "config.yaml" -o -name "*prow*" -type f 2>/dev/null | xargs cat 2>/dev/null | head -50Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2391
🌐 Web query:
Prow OWNERS file format numeric GitHub user ID support
💡 Result:
Prow’s OWNERS files do not have a special mode to identify people by numeric GitHub user ID. The format is YAML and the approvers / reviewers entries are parsed as strings (logins), i.e., GitHub usernames or aliases from OWNERS_ALIASES—not numeric IDs. [1], [2], [3]
If you put a number there, Prow will treat it as a login string (and it won’t map to a GitHub account unless a user literally has that as their login, which GitHub doesn’t). [2], [3]
Sources: Prow “Reviewers and Approvers” doc (OWNERS entries are GitHub usernames or aliases) [1]; Kubernetes OWNERS spec (GitHub usernames or aliases) [2]; Prow repoowners package types (Approvers []string, Reviewers []string) [3].
🌐 Web query:
Prow CI OWNERS file GitHub username vs user ID
💡 Result:
In Prow’s OWNERS files, you list GitHub usernames (logins/handles)—not GitHub numeric user IDs. You can also use aliases defined in OWNERS_ALIASES. GitHub team names are not supported by the Prow approve/owners mechanism. [1] [2]
Replace the numeric GitHub user ID with a username.
The entry "86254860" on lines 2 and 20 is a numeric GitHub user ID, but Prow's OWNERS files only support GitHub usernames (handles) or aliases defined in OWNERS_ALIASES. This numeric ID will not be recognized as a valid approver or reviewer. Replace it with the actual GitHub username for this account.
Note: The approvers and reviewers lists are identical, which is valid but consider whether you intend different privilege levels between the two groups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OWNERS` around lines 1 - 35, Replace the numeric GitHub ID "86254860" in the
OWNERS file (present under both approvers and reviewers) with the corresponding
GitHub username or an alias defined in OWNERS_ALIASES; update both occurrences
so Prow recognizes the account and keep the rest of the approvers/reviewers
entries unchanged.
|
Nice work on the 3-layer CI structure — the layered approach makes a lot of sense and the static validation targets are solid. One thing I wanted to flag after cross-referencing with the ticket: HYPERFLEET-525 describes Layer 2 as reusing the existing Was this a deliberate choice? Totally fine if so — Left a few smaller inline comments as well. |
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Thanks for putting this together! A few thoughts inline.
| cd $(TF_DIR) && terraform fmt -check -recursive -diff | ||
| cd $(TF_DIR) && terraform validate | ||
|
|
||
| .PHONY: lint-helm |
There was a problem hiding this comment.
The ticket describes Layer 2 as including terraform plan alongside the helm validation, but ci-dry-run doesn't depend on plan-terraform here. Should this be ci-dry-run: ci-validate plan-terraform validate-helm-charts? Or is plan intentionally left out since it needs real backend credentials and that defeats the "no credentials required" goal?
There was a problem hiding this comment.
plan is intentionally left out.
terraform plan requires GCS backend credentials for state initialization, which defeats the "no credentials required" goal for Layer 2. The terraform validation (terraform fmt -check and terraform validate) in Layer 1 catches syntax and configuration errors, and Layer 3 covers the full terraform apply path. So terraform plan is effectively redundant in our layered structure. Any suggestions?
| @@ -0,0 +1,35 @@ | |||
| approvers: | |||
| - "86254860" | |||
There was a problem hiding this comment.
This looks like a numeric GitHub ID rather than a username — Prow OWNERS files expect usernames so this entry will likely get ignored. Do you know which user this maps to?
There was a problem hiding this comment.
I've seen the discussion in Slack about this. We can update it once Dandan changes the username.
| @@ -157,7 +159,7 @@ install-api: check-helm check-kubectl check-namespace ## Install HyperFleet API | |||
| install-sentinel-clusters: check-helm check-kubectl check-namespace ## Install Sentinel for clusters | |||
There was a problem hiding this comment.
This will silently pass when shellcheck isn't installed, which is fine locally but could be a gap in CI. Might be worth adding a check for the $CI env var so it actually fails if shellcheck is missing in the CI environment, something like:
elif [ -n "$$CI" ]; then \
echo "ERROR: shellcheck is required in CI but not installed"; exit 1; \There was a problem hiding this comment.
Good suggestion. update in the latest commit.
| echo "WARN: shellcheck not installed, skipping"; \ | ||
| fi | ||
|
|
||
| .PHONY: ci-validate |
There was a problem hiding this comment.
Nit: this hardcodes -auto-approve while install-terraform up above uses $(AUTO_APPROVE). I'm guessing this is intentional for CI cleanup safety? If so, might be worth a quick comment here explaining that so it doesn't look like an oversight.
There was a problem hiding this comment.
Yes, intentional. CI cleanup (post step) must be non-interactive to prevent the job from hanging on a prompt. Added a comment in the latest commit to make this explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
348-354: Consider scoping health-check to specific deployment labels.Using
--allwaits for every pod in the namespace, which could be slow or flaky if unrelated pods exist. For more targeted checks, consider using label selectors for known HyperFleet/Maestro deployments.This is optional —
--allis acceptable for integration tests that fully control the namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 348 - 354, The health-check make target currently uses kubectl wait --for=condition=ready pods --all which can be slow or flaky; update the health-check target (and its two kubectl wait invocations) to use label selectors (kubectl wait ... --selector=... or -l ...) for your HyperFleet and Maestro deployments instead of --all, e.g. introduce variables like HYPERFLEET_LABEL and MAESTRO_LABEL (or reuse existing labels) and pass them to the kubectl wait commands so only the relevant pods are waited on.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 288-341: The validate-helm-charts target diverges from the
install-* targets and misses flags/values used in real deployments; update
validate-helm-charts to reuse the install-* targets with DRY_RUN=--dry-run (so
the exact install command set is validated) or, if you must keep helm template,
add the same flags and generated values: include --namespace $(NAMESPACE),
--kubeconfig $(KUBECONFIG) where appropriate, add --wait for charts that use it
(e.g., maestro), and pass --values $(GENERATED_DIR)/*.yaml / --set-file for
adapter configs so that validate-helm-charts and the install-* commands
(referenced by names like install-api, install-sentinel-clusters,
install-adapter1, etc.) render the same final manifests.
---
Nitpick comments:
In `@Makefile`:
- Around line 348-354: The health-check make target currently uses kubectl wait
--for=condition=ready pods --all which can be slow or flaky; update the
health-check target (and its two kubectl wait invocations) to use label
selectors (kubectl wait ... --selector=... or -l ...) for your HyperFleet and
Maestro deployments instead of --all, e.g. introduce variables like
HYPERFLEET_LABEL and MAESTRO_LABEL (or reuse existing labels) and pass them to
the kubectl wait commands so only the relevant pods are waited on.
| .PHONY: validate-helm-charts | ||
| validate-helm-charts: check-helm ## Render all Helm charts with helm template (no cluster required) | ||
| $(call set-chart-ref,$(HELM_DIR)/api,$(API_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/api | ||
| @echo "Validating api chart..." | ||
| helm template $(NAMESPACE)-api $(HELM_DIR)/api \ | ||
| $(if $(REGISTRY),--set hyperfleet-api.image.registry=$(REGISTRY)) \ | ||
| --set hyperfleet-api.image.tag=$(API_IMAGE_TAG) > /dev/null | ||
| $(call set-chart-ref,$(HELM_DIR)/sentinel-clusters,$(SENTINEL_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/sentinel-clusters | ||
| @echo "Validating sentinel-clusters chart..." | ||
| helm template $(NAMESPACE)-sentinel-clusters $(HELM_DIR)/sentinel-clusters \ | ||
| --set sentinel.broker.type=$(BROKER_TYPE) \ | ||
| $(if $(REGISTRY),--set sentinel.image.registry=$(REGISTRY)) \ | ||
| --set sentinel.image.tag=$(SENTINEL_IMAGE_TAG) > /dev/null | ||
| $(call set-chart-ref,$(HELM_DIR)/sentinel-nodepools,$(SENTINEL_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/sentinel-nodepools | ||
| @echo "Validating sentinel-nodepools chart..." | ||
| helm template $(NAMESPACE)-sentinel-nodepools $(HELM_DIR)/sentinel-nodepools \ | ||
| --set sentinel.broker.type=$(BROKER_TYPE) \ | ||
| $(if $(REGISTRY),--set sentinel.image.registry=$(REGISTRY)) \ | ||
| --set sentinel.image.tag=$(SENTINEL_IMAGE_TAG) > /dev/null | ||
| $(call set-chart-ref,$(HELM_DIR)/adapter1,$(ADAPTER_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/adapter1 | ||
| @echo "Validating adapter1 chart..." | ||
| helm template $(NAMESPACE)-adapter1 $(HELM_DIR)/adapter1 \ | ||
| --set hyperfleet-adapter.broker.type=$(BROKER_TYPE) \ | ||
| $(if $(REGISTRY),--set hyperfleet-adapter.image.registry=$(REGISTRY)) \ | ||
| --set hyperfleet-adapter.image.tag=$(ADAPTER_IMAGE_TAG) \ | ||
| --set-file hyperfleet-adapter.adapterConfig.yaml=$(HELM_DIR)/adapter1/adapter-config.yaml \ | ||
| --set-file hyperfleet-adapter.adapterTaskConfig.yaml=$(HELM_DIR)/adapter1/adapter-task-config.yaml > /dev/null | ||
| $(call set-chart-ref,$(HELM_DIR)/adapter2,$(ADAPTER_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/adapter2 | ||
| @echo "Validating adapter2 chart..." | ||
| helm template $(NAMESPACE)-adapter2 $(HELM_DIR)/adapter2 \ | ||
| --set hyperfleet-adapter.broker.type=$(BROKER_TYPE) \ | ||
| $(if $(REGISTRY),--set hyperfleet-adapter.image.registry=$(REGISTRY)) \ | ||
| --set hyperfleet-adapter.image.tag=$(ADAPTER_IMAGE_TAG) \ | ||
| --set-file hyperfleet-adapter.adapterConfig.yaml=$(HELM_DIR)/adapter2/adapter-config.yaml \ | ||
| --set-file hyperfleet-adapter.adapterTaskConfig.yaml=$(HELM_DIR)/adapter2/adapter-task-config.yaml > /dev/null | ||
| $(call set-chart-ref,$(HELM_DIR)/adapter3,$(ADAPTER_CHART_REF)) | ||
| helm dependency update $(HELM_DIR)/adapter3 | ||
| @echo "Validating adapter3 chart..." | ||
| helm template $(NAMESPACE)-adapter3 $(HELM_DIR)/adapter3 \ | ||
| --set hyperfleet-adapter.broker.type=$(BROKER_TYPE) \ | ||
| $(if $(REGISTRY),--set hyperfleet-adapter.image.registry=$(REGISTRY)) \ | ||
| --set hyperfleet-adapter.image.tag=$(ADAPTER_IMAGE_TAG) \ | ||
| --set-file hyperfleet-adapter.adapterConfig.yaml=$(HELM_DIR)/adapter3/adapter-config.yaml \ | ||
| --set-file hyperfleet-adapter.adapterTaskConfig.yaml=$(HELM_DIR)/adapter3/adapter-task-config.yaml > /dev/null | ||
| helm dependency update $(HELM_DIR)/maestro | ||
| @echo "Validating maestro chart..." | ||
| helm template $(MAESTRO_NS)-maestro $(HELM_DIR)/maestro \ | ||
| --set agent.messageBroker.mqtt.host=maestro-mqtt.$(MAESTRO_NS) > /dev/null | ||
| @echo "OK: all Helm charts rendered successfully" |
There was a problem hiding this comment.
validate-helm-charts diverges from install- targets — potential config drift.*
Per ciaranRoche's comment, the ticket specifies Layer 2 should reuse install-* targets with DRY_RUN=--dry-run to validate the exact configuration used in deployments. The current helm template approach omits several flags present in install-* targets:
Missing in helm template |
Present in install-* |
|---|---|
--namespace $(NAMESPACE) |
✓ |
--kubeconfig $(KUBECONFIG) |
✓ |
--wait |
✓ (maestro) |
--values $(GENERATED_DIR)/*.yaml |
✓ (sentinels, adapters) |
This means validation may pass while the actual deployment with generated values could fail.
Options to consider:
- Align with ticket: Replace
validate-helm-chartswith target that calls install-* targets usingDRY_RUN=--dry-run(requires cluster or stubbed kubeconfig) - Keep helm template (no cluster needed): Document the limitation and ensure generated values are also validated here
If option 2, consider adding the generated values to template commands where applicable:
helm template $(NAMESPACE)-sentinel-clusters $(HELM_DIR)/sentinel-clusters \
--set sentinel.broker.type=$(BROKER_TYPE) \
$(if $(REGISTRY),--set sentinel.image.registry=$(REGISTRY)) \
- --set sentinel.image.tag=$(SENTINEL_IMAGE_TAG) > /dev/null
+ --set sentinel.image.tag=$(SENTINEL_IMAGE_TAG) \
+ $(if $(wildcard $(GENERATED_DIR)/sentinel-clusters.yaml),--values $(GENERATED_DIR)/sentinel-clusters.yaml) > /dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 288 - 341, The validate-helm-charts target diverges
from the install-* targets and misses flags/values used in real deployments;
update validate-helm-charts to reuse the install-* targets with
DRY_RUN=--dry-run (so the exact install command set is validated) or, if you
must keep helm template, add the same flags and generated values: include
--namespace $(NAMESPACE), --kubeconfig $(KUBECONFIG) where appropriate, add
--wait for charts that use it (e.g., maestro), and pass --values
$(GENERATED_DIR)/*.yaml / --set-file for adapter configs so that
validate-helm-charts and the install-* commands (referenced by names like
install-api, install-sentinel-clusters, install-adapter1, etc.) render the same
final manifests.
Makefile
Outdated
| @echo "Installing Maestro..." | ||
|
|
||
| if ! helm upgrade --install $(MAESTRO_NS)-maestro $(HELM_DIR)/maestro \ | ||
| if ! helm upgrade --install $(if $(DRY_RUN),$(DRY_RUN)) $(MAESTRO_NS)-maestro $(HELM_DIR)/maestro \ |
There was a problem hiding this comment.
$(if $(DRY_RUN),$(DRY_RUN)) means we need to set DRY_RUN=--dry-run. Actually, the common usage is setting DRY_RUN=true, if that, the command will be incorrect. I advice to define a variable:
DRY_RUN_FLAG := $(if $(DRY_RUN),--dry-run)
There was a problem hiding this comment.
Updated to use DRY_RUN_FLAG :=
| helm dependency update $(HELM_DIR)/maestro | ||
| @echo "Validating maestro chart..." | ||
| helm template $(MAESTRO_NS)-maestro $(HELM_DIR)/maestro \ | ||
| --set agent.messageBroker.mqtt.host=maestro-mqtt.$(MAESTRO_NS) > /dev/null |
There was a problem hiding this comment.
- It is too hard to read the code, it is better to add a blank line between each component.
- There are too many duplicate code, it is better to define a function.
define validate-chart
$(call set-chart-ref,$(HELM_DIR)/$(1),$(2))
helm dependency update $(HELM_DIR)/$(1)
@echo "Validating $(1) chart..."
helm template $(NAMESPACE)-$(1) $(HELM_DIR)/$(1) $(3) > /dev/null
endef
.PHONY: validate-helm-charts
validate-helm-charts: check-helm ## Render all Helm charts with helm template
$(call validate-chart,api,$(API_CHART_REF),\
$(if $(REGISTRY),--set hyperfleet-api.image.registry=$(REGISTRY)) \
--set hyperfleet-api.image.tag=$(API_IMAGE_TAG))
$(call validate-chart,sentinel-clusters,$(SENTINEL_CHART_REF),\
--set sentinel.broker.type=$(BROKER_TYPE) \
$(if $(REGISTRY),--set sentinel.image.registry=$(REGISTRY)) \
--set sentinel.image.tag=$(SENTINEL_IMAGE_TAG))
# ... others
@echo "OK: all Helm charts rendered successfully"
There was a problem hiding this comment.
Updated. refactored using define validate-chart to reduce duplication
| SENTINEL_IMAGE_TAG ?= v0.1.1 | ||
| ADAPTER_IMAGE_TAG ?= v0.1.1 | ||
| DRY_RUN ?= | ||
| AUTO_APPROVE ?= |
There was a problem hiding this comment.
is that better set two to boolean rather than flag to fit for common usage?
There was a problem hiding this comment.
Updated. AUTO_APPROVE now accept boolean values
Makefile
Outdated
| validate-terraform: check-terraform ## Validate Terraform syntax and formatting | ||
| cd $(TF_DIR) && terraform init -backend=false | ||
| cd $(TF_DIR) && terraform fmt -check -recursive -diff | ||
| cd $(TF_DIR) && terraform validate |
There was a problem hiding this comment.
A better way:
cd $(TF_DIR) && \
terraform init -backend=false && \
terraform fmt -check -recursive -diff && \
terraform validate
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Makefile (1)
292-342:⚠️ Potential issue | 🟠 MajorLayer-2 Helm validation still diverges from install inputs (config drift risk).
validate-helm-chartsstill renders viahelm templatewith a different argument set than install targets, and it omits generated values files used during real installs (for sentinel/adapters). This can pass CI while deploy-time installs fail with actual generated inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 292 - 342, The CI Helm validation (validate-helm-charts / validate-chart) is rendering charts with different args than real installs and omits generated values files, risking config drift; update validate-chart and its calls in validate-helm-charts to pass the exact same --set/--set-file inputs used by install targets (include the adapterX/adapter-config.yaml and adapter-task-config.yaml files, any generated sentinel values files, registry/tag flags, and the same namespace flags), and ensure the helm template invocation in validate-chart mirrors the install invocation (same namespace/name, same file paths), using the existing set-chart-ref helper so validation uses the exact generated inputs used at deploy time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.ci`:
- Line 3: The Dockerfile currently leaves the final image running as root (the
"USER root" instruction); change this by creating a non-root runtime user and
switching to it at the end of the Dockerfile: add a RUN that creates a
user/group (e.g., groupadd/useradd or adduser), ensure any runtime-owned
files/directories are chowned to that user during the root phase, and replace
the final "USER root" with "USER <nonroot-username>" so the container runs as
the non-root user after provisioning. Ensure the created username matches any
entrypoint/permission assumptions.
- Around line 9-16: Pin and verify all downloaded artifacts: replace the dynamic
get-helm.sh download with a specific Helm release URL (or vendored installer)
and verify its SHA256 before running; keep TERRAFORM_VERSION but add an explicit
expected SHA256 for the terraform zip and verify it after download; replace the
kubectl stable.txt resolution with a pinned kubectl version and verify the
binary SHA256 after download; and install the helm-git plugin using a specific
release tag (not HEAD). Update the RUN lines that reference get_helm.sh,
TERRAFORM_VERSION and the kubectl curl, and the helm plugin install command to
perform versioned fetches and checksum checks prior to installation.
In `@Makefile`:
- Around line 22-23: DRY_RUN_FLAG and AUTO_APPROVE_FLAG currently treat any
non-empty value (including "false") as true; change their assignments to
explicitly test for allowed truthy values. Replace the current expansions with a
filter-based check, e.g. use $(if $(filter true 1,$(DRY_RUN)),--dry-run,) and
$(if $(filter true 1,$(AUTO_APPROVE)),-auto-approve,) so only "true" or "1"
enable DRY_RUN_FLAG and AUTO_APPROVE_FLAG (update both DRY_RUN_FLAG and
AUTO_APPROVE_FLAG definitions).
---
Duplicate comments:
In `@Makefile`:
- Around line 292-342: The CI Helm validation (validate-helm-charts /
validate-chart) is rendering charts with different args than real installs and
omits generated values files, risking config drift; update validate-chart and
its calls in validate-helm-charts to pass the exact same --set/--set-file inputs
used by install targets (include the adapterX/adapter-config.yaml and
adapter-task-config.yaml files, any generated sentinel values files,
registry/tag flags, and the same namespace flags), and ensure the helm template
invocation in validate-chart mirrors the install invocation (same
namespace/name, same file paths), using the existing set-chart-ref helper so
validation uses the exact generated inputs used at deploy time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8278c4a-1f64-4362-b212-50b7f0289b01
📒 Files selected for processing (2)
Dockerfile.ciMakefile
| @@ -0,0 +1,16 @@ | |||
| FROM src | |||
| WORKDIR /go/src/github.com/openshift-hyperfleet/hyperfleet-infra | |||
| USER root | |||
There was a problem hiding this comment.
Run CI jobs as non-root after provisioning steps.
Line 3 keeps the final image user as root, which increases impact if a presubmit script/dependency is compromised. Install as root, then switch to a non-root runtime user before the final layer.
🧰 Tools
🪛 Trivy (0.69.3)
[error] 3-3: Image user should not be 'root'
Last USER command in Dockerfile should not be 'root'
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile.ci` at line 3, The Dockerfile currently leaves the final image
running as root (the "USER root" instruction); change this by creating a
non-root runtime user and switching to it at the end of the Dockerfile: add a
RUN that creates a user/group (e.g., groupadd/useradd or adduser), ensure any
runtime-owned files/directories are chowned to that user during the root phase,
and replace the final "USER root" with "USER <nonroot-username>" so the
container runs as the non-root user after provisioning. Ensure the created
username matches any entrypoint/permission assumptions.
| RUN curl -fsSL -o /tmp/get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 \ | ||
| && chmod +x /tmp/get_helm.sh && /tmp/get_helm.sh && rm /tmp/get_helm.sh | ||
| RUN TERRAFORM_VERSION=1.9.8 \ | ||
| && curl -fsSL -o /tmp/terraform.zip "https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_linux_amd64.zip" \ | ||
| && unzip -o /tmp/terraform.zip -d /usr/local/bin/ && rm /tmp/terraform.zip | ||
| RUN curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" \ | ||
| && chmod 755 kubectl && mv kubectl /usr/local/bin/ | ||
| RUN helm plugin install https://github.com/aslafy-z/helm-git |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the Dockerfile.ci file
find . -name "Dockerfile.ci" -type fRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 96
🏁 Script executed:
# Read the Dockerfile.ci to examine lines 9-16
cat -n ./Dockerfile.ci | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 1062
🏁 Script executed:
# Check if the file references are as described - verify the mutable nature of the URLs
# Test if stable.txt resolves dynamically
curl -s -L https://dl.k8s.io/release/stable.txt | head -1
# Check if get.helm.sh is available
curl -s -I https://get.helm.sh/helm-v3.17.3-linux-amd64.tar.gz | head -5Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 234
🏁 Script executed:
# Verify the get-helm-3 script is mutable (from main branch)
curl -s -I https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 | head -5
# Check if helm-git accepts version flags
curl -s https://api.github.com/repos/aslafy-z/helm-git/releases/latest | grep -o '"tag_name": "[^"]*"'
# Verify if the suggested versions are reasonable by checking terraform releases
curl -s https://api.hashicorp.com/v1/releases/terraform?limit=5 | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 338
🏁 Script executed:
# Check the specific versions suggested in the diff
# Verify helm release history for v3.17.3
curl -s https://api.github.com/repos/helm/helm/releases | grep '"tag_name"' | head -10
# Verify kubectl versions
curl -s https://api.github.com/repos/kubernetes/kubernetes/releases | grep '"tag_name"' | head -10 | grep -E "v1\.31|v1\.32|v1\.33|v1\.35"Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 518
Pin and verify downloaded toolchain artifacts.
Lines 9-16 pull mutable upstream content without integrity checks, creating a supply-chain and reproducibility risk for CI:
get-helm.shfrom themainbranch changes over timekubectlresolved viastable.txt(currently v1.35.2, changes daily)helm-gitplugin installed from HEAD without version specification
Replace with explicit version pins and SHA256 verification:
Suggested hardening pattern
+ARG HELM_VERSION=v3.20.1
+ARG TERRAFORM_VERSION=1.9.8
+ARG KUBECTL_VERSION=v1.35.2
+ARG HELM_GIT_VERSION=v1.5.2
+# Add SHA256 args for each binary
-RUN curl -fsSL -o /tmp/get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 \
- && chmod +x /tmp/get_helm.sh && /tmp/get_helm.sh && rm /tmp/get_helm.sh
+RUN curl -fsSLo /tmp/helm.tgz "https://get.helm.sh/helm-${HELM_VERSION}-linux-amd64.tar.gz" \
+ && echo "${HELM_SHA256} /tmp/helm.tgz" | sha256sum -c - \
+ && tar -xzf /tmp/helm.tgz -C /tmp && mv /tmp/linux-amd64/helm /usr/local/bin/helm \
+ && rm -rf /tmp/helm.tgz /tmp/linux-amd64
-RUN curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" \
- && chmod 755 kubectl && mv kubectl /usr/local/bin/
+RUN curl -fsSLo /tmp/kubectl "https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/linux/amd64/kubectl" \
+ && echo "${KUBECTL_SHA256} /tmp/kubectl" | sha256sum -c - \
+ && chmod 755 /tmp/kubectl && mv /tmp/kubectl /usr/local/bin/kubectl
-RUN helm plugin install https://github.com/aslafy-z/helm-git
+RUN helm plugin install https://github.com/aslafy-z/helm-git --version ${HELM_GIT_VERSION}Use current release versions and generate SHA256 checksums from official release artifacts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile.ci` around lines 9 - 16, Pin and verify all downloaded artifacts:
replace the dynamic get-helm.sh download with a specific Helm release URL (or
vendored installer) and verify its SHA256 before running; keep TERRAFORM_VERSION
but add an explicit expected SHA256 for the terraform zip and verify it after
download; replace the kubectl stable.txt resolution with a pinned kubectl
version and verify the binary SHA256 after download; and install the helm-git
plugin using a specific release tag (not HEAD). Update the RUN lines that
reference get_helm.sh, TERRAFORM_VERSION and the kubectl curl, and the helm
plugin install command to perform versioned fetches and checksum checks prior to
installation.
| DRY_RUN_FLAG := $(if $(DRY_RUN),--dry-run) | ||
| AUTO_APPROVE_FLAG := $(if $(AUTO_APPROVE),-auto-approve) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -50 Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2290
🏁 Script executed:
sed -n '20,35p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 827
🏁 Script executed:
sed -n '225,235p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 622
🏁 Script executed:
sed -n '290,350p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 3524
🏁 Script executed:
grep -n "install-api\|install-sentinel\|install-adapter" Makefile | head -20Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 1385
🏁 Script executed:
sed -n '150,250p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 6407
🏁 Script executed:
rg "GENERATED_DIR" Makefile -A 2 -B 2Repository: openshift-hyperfleet/hyperfleet-infra
Length of output: 2297
🏁 Script executed:
sed -n '260,290p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 1377
🏁 Script executed:
sed -n '225,235p' Makefile | cat -nRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 622
🏁 Script executed:
wc -l MakefileRepository: openshift-hyperfleet/hyperfleet-infra
Length of output: 93
AUTO_APPROVE=false and DRY_RUN=false still enable flags.
Using $(if $(VAR),...) makes any non-empty value truthy. Setting false still triggers --dry-run and -auto-approve; line 231 can auto-approve Terraform unexpectedly.
Safer truthy parsing
+TRUTHY_VALUES := 1 true TRUE yes YES y Y on ON
-DRY_RUN_FLAG := $(if $(DRY_RUN),--dry-run)
-AUTO_APPROVE_FLAG := $(if $(AUTO_APPROVE),-auto-approve)
+DRY_RUN_FLAG := $(if $(filter $(TRUTHY_VALUES),$(strip $(DRY_RUN))),--dry-run)
+AUTO_APPROVE_FLAG := $(if $(filter $(TRUTHY_VALUES),$(strip $(AUTO_APPROVE))),-auto-approve)Also applies to: 231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 22 - 23, DRY_RUN_FLAG and AUTO_APPROVE_FLAG currently
treat any non-empty value (including "false") as true; change their assignments
to explicitly test for allowed truthy values. Replace the current expansions
with a filter-based check, e.g. use $(if $(filter true 1,$(DRY_RUN)),--dry-run,)
and $(if $(filter true 1,$(AUTO_APPROVE)),-auto-approve,) so only "true" or "1"
enable DRY_RUN_FLAG and AUTO_APPROVE_FLAG (update both DRY_RUN_FLAG and
AUTO_APPROVE_FLAG definitions).
| jq \ | ||
| unzip \ | ||
| && yum clean all | ||
| RUN curl -fsSL -o /tmp/get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 \ |
There was a problem hiding this comment.
Priority: Security
Terraform is pinned at 1.9.8 on line 11 — nice. But helm (line 9) and kubectl (line 14) aren't
pinned, so rebuilding this image next week could give you completely different versions.
For helm, the get-helm-3 script supports a DESIRED_VERSION env var:
RUN HELM_VERSION=v3.17.3 \
&& curl -fsSL -o /tmp/get_helm.sh
https://raw.githubusercontent.com/helm/helm/v${HELM_VERSION}/scripts/get-helm-3 \
&& chmod +x /tmp/get_helm.sh && DESIRED_VERSION=${HELM_VERSION} /tmp/get_helm.sh && rm
/tmp/get_helm.sh(Note: also pinning the script download to the tag instead of main.)
For kubectl, pin similarly:
RUN KUBECTL_VERSION=v1.32.3 \
&& curl -LO "https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/linux/amd64/kubectl" \
&& chmod 755 kubectl && mv kubectl /usr/local/bin/4 participants
Summary by CodeRabbit
New Features
Documentation
CI / Chores
Chores