OAPE-878: Add OpenSpec harness-evals docs and coding guidelines - #465
OAPE-878: Add OpenSpec harness-evals docs and coding guidelines#465arun717 wants to merge 5 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
arun717
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@arun717: This pull request references OAPE-878 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 task 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. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds repository guidance and extensive Cert Manager Operator documentation. It covers architecture, APIs, controllers, security, OpenShift integration, FIPS, packaging, development, testing, references, ADRs, and Harness evaluation metadata. It also corrects unit-test package filtering. ChangesOperator architecture, APIs, and controller contracts
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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: 8
🧹 Nitpick comments (2)
ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md (1)
10-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the redundant
make generatecommand.
make buildalready runsgenerate,fmt, andvet.make verifyrunsfmtandvetagain. Usemake build && make update-manifests && make verifyfor this workflow.🤖 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 `@ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md` around lines 10 - 15, Update the documented development workflow by removing the redundant make generate command from the command chain, leaving make build && make update-manifests && make verify because make build already performs generation.docs/security-guidelines.md (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReword to remove the repeated "only".
The sentence at line 23 uses "only" twice close together: "are not user-configurable ... only expose an
egress-only API surface for new NetworkPolicy config." Rephrase to remove the repetition and make the rule clearer.✏️ Proposed wording fix
- ports it already runs (`cert_manager_networkpolicy.go`); only expose an `egress`-only API surface for - new NetworkPolicy config. + ports it already runs (`cert_manager_networkpolicy.go`); expose only an `egress` API surface for + new NetworkPolicy config.🤖 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 `@docs/security-guidelines.md` around lines 22 - 24, Reword the NetworkPolicy guidance near the ingress rule so “only” is not repeated in the same sentence. Preserve the meaning that ingress is derived by the operator and that new configuration exposes an egress-only API surface, using clearer phrasing around the existing cert_manager_networkpolicy.go reference.Source: Linters/SAST tools
🤖 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 `@ai-docs/architecture/components.md`:
- Around line 39-40: The FeatureGate error behavior is inconsistent across the
documentation; align all three references with setupFeatureGates: retry
discovery failures, fail closed by treating unavailable gates as disabled, and
keep the operator running without starting optional controllers until gates are
known enabled. Update ai-docs/architecture/components.md lines 39-40,
ai-docs/decisions/adr-0003-feature-gates.md line 18, and
docs/operator-controllers-guidelines.md lines 164-166 to describe this same
persistent-error behavior.
In `@docs/api-contracts-guidelines.md`:
- Around line 29-31: The documentation contradicts itself: lines 29-31 instruct
contributors to add operand CRD YAML under config/crd/bases/, while lines
157-160 prohibit manual edits to all files in that directory. Clarify the
guidance by identifying which files in config/crd/bases/ are controller-gen
output and which are operand source manifests, then update both sections to
specify the correct source path for operand CRDs and document the generation
command that produces auto-generated CRD files so contributors understand where
to place manual changes versus what gets regenerated.
In `@docs/error-handling-guidelines.md`:
- Line 4: The link fragment in the reference on line 4
(`#7-library-go-vs-ctrl-runtime-do-not-mix-patterns`) does not match the anchor
generated by the corresponding heading on line 140. Verify the actual anchor
that the heading generates (based on markdown anchor generation rules) and
update the link fragment in the reference on line 4 to match it correctly, or
alternatively add an explicit anchor attribute to the heading on line 140 to
match the expected fragment.
In `@docs/olm-packaging-guidelines.md`:
- Around line 137-148: Align the “Verify scripts” table and nearby instructions
with the existing bindata verification flow: add the bindata verifier and its
`make verify-bindata` target to the table, or explicitly instruct authors to run
`make verify-bindata` directly rather than implying `make verify-scripts` covers
it. Preserve the existing CRD guidance.
- Around line 84-87: The packaging guidelines should distinguish generated
bundle metadata validation from runtime image-map validation: require
operator-sdk bundle validate for relatedImages, plus source-level or unit-test
coverage confirming every RELATED_IMAGE_* variable has a corresponding
imageEnvMap (or equivalent constants) mapping. Update the guidance around adding
new operand images without implying bundle validation checks Go runtime
mappings.
- Around line 57-62: Update the olm.skipRange guidance in the packaging
guidelines so z-stream releases use the candidate version as the exclusive upper
bound (for example, <1.20.1 for 1.20.1), while minor releases use the next
minor’s .0 bound (for example, <1.21.0 for 1.21.0). Preserve the existing
lower-bound and replaces guidance.
In `@docs/testing-guidelines.md`:
- Around line 27-30: Update the test-unit documentation and corresponding
Makefile exclusion rule to use directory alternatives rather than a character
class, matching only test/e2e, test/apis, and test/utils with a following slash
or end of path. Keep the listed test-unit behavior consistent with the Makefile.
- Around line 81-85: Update the CtrlClient example in the testing guidelines to
use valid Go interface syntax with complete method signatures and
parameter/return types, or clearly mark the example as pseudocode using a non-Go
code fence or prose. Preserve the counterfeiter directives only if the example
remains intended to be executable Go.
---
Nitpick comments:
In `@ai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.md`:
- Around line 10-15: Update the documented development workflow by removing the
redundant make generate command from the command chain, leaving make build &&
make update-manifests && make verify because make build already performs
generation.
In `@docs/security-guidelines.md`:
- Around line 22-24: Reword the NetworkPolicy guidance near the ingress rule so
“only” is not repeated in the same sentence. Preserve the meaning that ingress
is derived by the operator and that new configuration exposes an egress-only API
surface, using clearer phrasing around the existing
cert_manager_networkpolicy.go reference.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc6081ff-90f9-41fd-bca6-bd299ff4168b
📒 Files selected for processing (22)
AGENTS.mdai-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.mdai-docs/CERT_MANAGER_OPERATOR_TESTING.mdai-docs/architecture/components.mdai-docs/decisions/adr-0001-dual-controller-frameworks.mdai-docs/decisions/adr-0002-apply-strategies.mdai-docs/decisions/adr-0003-feature-gates.mdai-docs/decisions/adr-template.mdai-docs/domain/certmanager.mdai-docs/domain/istiocsr.mdai-docs/domain/trustmanager.mdai-docs/exec-plans/README.mdai-docs/references/ecosystem.mdai-docs/references/enhancements.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/fips-guidelines.mddocs/integration-guidelines.mddocs/olm-packaging-guidelines.mddocs/operator-controllers-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
arun717
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
harness-evals/harness-docs/error-handling-guidelines.md (1)
39-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the
ServiceUnavailableguidance consistent.The table on Lines 39-42 classifies
IsServiceUnavailableasIrrecoverableError, but the rule of thumb on Lines 45-48 describes only permission and validation errors as irrecoverable. IncludeServiceUnavailablein the rule, or change the mapping if it is not intended.🤖 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 `@harness-evals/harness-docs/error-handling-guidelines.md` around lines 39 - 48, Update the rule-of-thumb guidance to explicitly include ServiceUnavailable-shaped API errors among the irrecoverable cases, keeping it consistent with the existing IsServiceUnavailable → IrrecoverableError mapping and leaving the remaining retry classification unchanged.harness-evals/harness-docs/testing-guidelines.md (1)
144-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDistinguish CertManager name handling.
CertManageraccepts non-standard names but processes onlyLister().Get("cluster"); such CRs are silently ignored.TrustManagerandIstioCSRreject non-standard names through CEL validation. Update line 144 and keepharness-evals/harness-docs/domain/certmanager.mdconsistent.🤖 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 `@harness-evals/harness-docs/testing-guidelines.md` at line 144, Update the operator CR naming guidance to distinguish CertManager from TrustManager and IstioCSR: state that CertManager accepts non-standard names but only processes the singleton named "cluster", while TrustManager and IstioCSR reject non-standard names through CEL validation. Apply the same correction in the CertManager documentation under its domain guide and keep the production-matching singleton guidance accurate.
♻️ Duplicate comments (3)
harness-evals/harness-docs/olm-packaging-guidelines.md (3)
137-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the actual bindata verification target.
Line [130] defines
make verify-bindata, but Line [154] tells authors to confirmhack/verify-bindatathroughmake verify-scripts. The verification table also does not list bindata verification. Replace the checklist text withmake verify-bindata, and state explicitly whether that target is included inmake verify-scripts.Also applies to: 152-156
🤖 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 `@harness-evals/harness-docs/olm-packaging-guidelines.md` around lines 137 - 148, The verification guidance around the “Verify scripts” table and its follow-up checklist uses the wrong bindata command. Add the bindata verification entry using the actual make verify-bindata target, explicitly mark whether it is included in make verify-scripts, and update the checklist text to instruct authors to run make verify-bindata rather than a hack/verify-bindata script.
57-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a release-type-specific
olm.skipRangeupper bound.The rule at Line [60] applies
<X.Y.0>to both minor and z-stream releases. For1.20.1,<1.20.0>excludes1.20.0, so that installed version cannot use the skip range. Use<1.20.1>for1.20.1and<1.21.0>for1.21.0. Update the checklist at Line [166] to state this rule.Also applies to: 165-169
🤖 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 `@harness-evals/harness-docs/olm-packaging-guidelines.md` around lines 57 - 62, Update the olm.skipRange guidance in the release checklist and its corresponding checklist entry to use the release-specific upper bound: for z-stream releases such as 1.20.1, use the new version threshold (e.g., <1.20.1), while for minor releases such as 1.21.0, use the next minor baseline (e.g., <1.21.0). Preserve the existing lower-bound and channel-label guidance.
84-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeparate bundle validation from runtime image-map validation.
Lines [87] and [175] claim that
operator-sdk bundle validatevalidates the Go runtime image map. It validates generated bundle metadata. It does not prove that eachRELATED_IMAGE_*variable is consumed by the correct resolver. The table identifiespkg/controller/certmanager/related_images.goas the CertManager consumer, but Line [173] names onlypkg/controller/*/constants.go. Document the component-specific resolver and require source-level or unit-test coverage for the mapping. A missing runtime mapping can pass bundle validation and still break operand image resolution in disconnected deployments.Also applies to: 171-175
🤖 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 `@harness-evals/harness-docs/olm-packaging-guidelines.md` around lines 84 - 87, Update the operand-image validation guidance to distinguish generated bundle metadata validation from runtime resolver validation. In the related-image requirements and table, identify each component’s actual resolver, including pkg/controller/certmanager/related_images.go for CertManager rather than only pkg/controller/*/constants.go, and require source-level or unit-test coverage proving each RELATED_IMAGE_* variable maps to the correct runtime image.
🤖 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 `@harness-evals/evals/plan_eval.yaml`:
- Line 2: Create the three missing evaluation template files:
harness-evals/evals/templates/plan-template.md for plan_eval.yaml,
harness-evals/evals/templates/repo-assessment-template.md for
repo-assessment_eval.yaml, and harness-evals/evals/templates/tasks-template.md
for tasks_eval.yaml. Populate each with the content expected by its
corresponding evaluation stage so all three template references resolve
successfully.
In `@harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md`:
- Line 15: Update the controller startup description to reference the resolved
feature-gate state rather than only explicit --unsupported-addon-features
opt-in. In the ADR sentence covering IstioCSR and TrustManager, state that each
controller starts when its corresponding resolved gate is enabled, and identify
IstioCSR as enabled by default.
In `@harness-evals/harness-docs/olm-packaging-guidelines.md`:
- Line 144: Update the CRD verifier command references in the table to
consistently use the repository-root paths with the hack/ prefix, including
verify-crds-version-upgrade.sh at the referenced entries. Preserve the existing
command descriptions and direct-execution guidance.
In `@harness-evals/harness-docs/security-guidelines.md`:
- Around line 110-112: Update the trusted-CA missing ConfigMap guidance near the
hook’s retryable error description to state that the library-go
DeploymentController surfaces the transient condition as Degraded while retrying
via its rate limiter, and that it self-clears when the ConfigMap appears; remove
the instruction not to convert it to Degraded.
---
Outside diff comments:
In `@harness-evals/harness-docs/error-handling-guidelines.md`:
- Around line 39-48: Update the rule-of-thumb guidance to explicitly include
ServiceUnavailable-shaped API errors among the irrecoverable cases, keeping it
consistent with the existing IsServiceUnavailable → IrrecoverableError mapping
and leaving the remaining retry classification unchanged.
In `@harness-evals/harness-docs/testing-guidelines.md`:
- Line 144: Update the operator CR naming guidance to distinguish CertManager
from TrustManager and IstioCSR: state that CertManager accepts non-standard
names but only processes the singleton named "cluster", while TrustManager and
IstioCSR reject non-standard names through CEL validation. Apply the same
correction in the CertManager documentation under its domain guide and keep the
production-matching singleton guidance accurate.
---
Duplicate comments:
In `@harness-evals/harness-docs/olm-packaging-guidelines.md`:
- Around line 137-148: The verification guidance around the “Verify scripts”
table and its follow-up checklist uses the wrong bindata command. Add the
bindata verification entry using the actual make verify-bindata target,
explicitly mark whether it is included in make verify-scripts, and update the
checklist text to instruct authors to run make verify-bindata rather than a
hack/verify-bindata script.
- Around line 57-62: Update the olm.skipRange guidance in the release checklist
and its corresponding checklist entry to use the release-specific upper bound:
for z-stream releases such as 1.20.1, use the new version threshold (e.g.,
<1.20.1), while for minor releases such as 1.21.0, use the next minor baseline
(e.g., <1.21.0). Preserve the existing lower-bound and channel-label guidance.
- Around line 84-87: Update the operand-image validation guidance to distinguish
generated bundle metadata validation from runtime resolver validation. In the
related-image requirements and table, identify each component’s actual resolver,
including pkg/controller/certmanager/related_images.go for CertManager rather
than only pkg/controller/*/constants.go, and require source-level or unit-test
coverage proving each RELATED_IMAGE_* variable maps to the correct runtime
image.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 037b908f-184b-4e66-b2e4-ca83d37aee44
📒 Files selected for processing (26)
AGENTS.mdharness-evals/evals/code-generation_eval.yamlharness-evals/evals/plan_eval.yamlharness-evals/evals/repo-assessment_eval.yamlharness-evals/evals/tasks_eval.yamlharness-evals/harness-docs/CERT_MANAGER_OPERATOR_DEVELOPMENT.mdharness-evals/harness-docs/CERT_MANAGER_OPERATOR_TESTING.mdharness-evals/harness-docs/api-contracts-guidelines.mdharness-evals/harness-docs/architecture/components.mdharness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.mdharness-evals/harness-docs/decisions/adr-0002-apply-strategies.mdharness-evals/harness-docs/decisions/adr-0003-feature-gates.mdharness-evals/harness-docs/decisions/adr-template.mdharness-evals/harness-docs/domain/certmanager.mdharness-evals/harness-docs/domain/istiocsr.mdharness-evals/harness-docs/domain/trustmanager.mdharness-evals/harness-docs/error-handling-guidelines.mdharness-evals/harness-docs/exec-plans/README.mdharness-evals/harness-docs/fips-guidelines.mdharness-evals/harness-docs/integration-guidelines.mdharness-evals/harness-docs/olm-packaging-guidelines.mdharness-evals/harness-docs/operator-controllers-guidelines.mdharness-evals/harness-docs/references/ecosystem.mdharness-evals/harness-docs/references/enhancements.mdharness-evals/harness-docs/security-guidelines.mdharness-evals/harness-docs/testing-guidelines.md
| @@ -0,0 +1,3 @@ | |||
| stage: plan | |||
| template: templates/plan-template.md | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for spec in \
harness-evals/evals/plan_eval.yaml \
harness-evals/evals/repo-assessment_eval.yaml \
harness-evals/evals/tasks_eval.yaml
do
template="$(awk -F': ' '$1 == "template" { print $2; exit }' "$spec")"
config_dir="$(dirname "$spec")"
candidate="$config_dir/$template"
if [[ -f "$candidate" ]]; then
printf 'FOUND: %s -> %s\n' "$spec" "$candidate"
else
printf 'MISSING relative to config: %s -> %s\n' "$spec" "$candidate"
fi
done
printf '\nTemplate files:\n'
fd -t f -a '(plan-template|repo-assessment-template|tasks-template)\.md$' .
printf '\nResolver references:\n'
rg -n -C 3 'template:|plan_eval|repo-assessment_eval|tasks_eval|templates/' .Repository: openshift/cert-manager-operator
Length of output: 50389
Create the missing evaluation template files.
The three evaluation stages cannot load without their template files. All three templates are missing from the repository:
harness-evals/evals/templates/plan-template.mdis missing (referenced byplan_eval.yaml)harness-evals/evals/templates/repo-assessment-template.mdis missing (referenced byrepo-assessment_eval.yaml)harness-evals/evals/templates/tasks-template.mdis missing (referenced bytasks_eval.yaml)
Create these files in the harness-evals/evals/templates/ directory with the content each evaluation stage expects.
📍 Affects 3 files
harness-evals/evals/plan_eval.yaml#L2-L2(this comment)harness-evals/evals/repo-assessment_eval.yaml#L2-L2harness-evals/evals/tasks_eval.yaml#L2-L2
🤖 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 `@harness-evals/evals/plan_eval.yaml` at line 2, Create the three missing
evaluation template files: harness-evals/evals/templates/plan-template.md for
plan_eval.yaml, harness-evals/evals/templates/repo-assessment-template.md for
repo-assessment_eval.yaml, and harness-evals/evals/templates/tasks-template.md
for tasks_eval.yaml. Populate each with the content expected by its
corresponding evaluation stage so all three template references resolve
successfully.
| creates or labels this ConfigMap itself (`../../docs/proxy.md`). If a bundle isn't found yet, the hook | ||
| returns a retryable error (`(Retrying) trusted CA config map %q doesn't exist`); don't convert this | ||
| to a fatal/Degraded condition. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'withCAConfigMap|trusted CA config map|Degraded|rate limiter' pkg/controller harness-evals/harness-docsRepository: openshift/cert-manager-operator
Length of output: 50389
Update security-guidelines.md to align with the documented trusted-CA error-handling behavior.
integration-guidelines.md line 15 documents that when withCAConfigMap detects a missing ConfigMap, it returns a retryable error. The library-go DeploymentController treats this error by setting Degraded=True and retrying via the rate limiter until the ConfigMap appears. This is not a fatal failure—it is a transient degradation that self-clears when the dependency resolves.
The text in lines 110–112 of security-guidelines.md states "don't convert this to a fatal/Degraded condition," which contradicts the documented library-go behavior. Clarify that a retryable missing-dependency error surfaces as Degraded while retrying, not as a permanent failure, or update security-guidelines.md to match the documented behavior in integration-guidelines.md.
🤖 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 `@harness-evals/harness-docs/security-guidelines.md` around lines 110 - 112,
Update the trusted-CA missing ConfigMap guidance near the hook’s retryable error
description to state that the library-go DeploymentController surfaces the
transient condition as Degraded while retrying via its rate limiter, and that it
self-clears when the ConfigMap appears; remove the instruction not to convert it
to Degraded.
0636f65 to
fe9e3fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/download-tools.sh`:
- Around line 22-24: Update the CURL_OPTS definition in download-tools.sh to
include --proto '=https' and --proto-redir '=https', ensuring every curl
invocation for artifacts and checksums restricts both initial requests and
redirects to HTTPS.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b46f4ac-400c-4388-8533-5350c653576f
📒 Files selected for processing (1)
hack/download-tools.sh
| # curl options: fail on HTTP errors, follow redirects, retry transient | ||
| # failures (including DNS/connect) common in CI when hitting GitHub. | ||
| CURL_OPTS=(--fail --silent --show-error --location --retry 5 --retry-delay 2 --retry-all-errors) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- downloader structure ---'
sed -n '1,180p' hack/download-tools.sh
printf '%s\n' '--- curl availability and options ---'
if command -v curl >/dev/null 2>&1; then
curl --version | head -n 2
curl --help all 2>/dev/null | grep -E -- '--proto(-redir)?|retry-all-errors' || true
else
echo 'curl not available'
fi
printf '%s\n' '--- curl references ---'
rg -n -- '--proto-redir|--proto|--location|retry-all-errors|CURL_OPTS|verify_checksum' hack Makefile .github 2>/dev/null || trueRepository: openshift/cert-manager-operator
Length of output: 7381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact numbered locations ---'
nl -ba hack/download-tools.sh | sed -n '18,32p;76,165p'
printf '%s\n' '--- all downloader option and call references ---'
rg -n -C 2 'CURL_OPTS|curl .*--location|curl .*--proto' hack/download-tools.sh
printf '%s\n' '--- curl protocol behavior ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
printf 'https://example.invalid\n' > "$tmpdir/url"
curl --help all 2>/dev/null | grep -E -- '--proto(-redir)?|retry-all-errors'
printf '%s\n' 'Expected option forms:'
printf '%s\n' "--proto '=https'"
printf '%s\n' "--proto-redir '=https'"Repository: openshift/cert-manager-operator
Length of output: 1771
Security Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: External · Exploitability: Difficult
Restrict redirects to HTTPS in every download.
hack/download-tools.sh calls curl --silent --location directly for all six artifact and checksum downloads. Add --proto '=https' --proto-redir '=https' to each call, or define and use CURL_OPTS. Otherwise, an HTTPS-to-HTTP redirect can allow replacement of both the binary and its checksum before verify_checksum.
🤖 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/download-tools.sh` around lines 22 - 24, Update the CURL_OPTS definition
in download-tools.sh to include --proto '=https' and --proto-redir '=https',
ensuring every curl invocation for artifacts and checksums restricts both
initial requests and redirects to HTTPS.
|
@arun717 please move contents of |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@harness-evals/harness-docs/api-contracts-guidelines.md`:
- Around line 29-34: Update the operand CRD guidance to include
hack/update-istio-csr-manifests.sh alongside the existing cert-manager and
trust-manager manifest updaters, while preserving the make update-manifests and
kustomization wiring instructions.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ced8beb5-05e8-4d07-afee-35fa8223ad1d
📒 Files selected for processing (9)
Makefileharness-evals/harness-docs/api-contracts-guidelines.mdharness-evals/harness-docs/architecture/components.mdharness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.mdharness-evals/harness-docs/decisions/adr-0003-feature-gates.mdharness-evals/harness-docs/error-handling-guidelines.mdharness-evals/harness-docs/olm-packaging-guidelines.mdharness-evals/harness-docs/security-guidelines.mdharness-evals/harness-docs/testing-guidelines.md
🚧 Files skipped from review as they are similar to previous changes (7)
- harness-evals/harness-docs/decisions/adr-0001-dual-controller-frameworks.md
- harness-evals/harness-docs/decisions/adr-0003-feature-gates.md
- harness-evals/harness-docs/error-handling-guidelines.md
- harness-evals/harness-docs/security-guidelines.md
- harness-evals/harness-docs/testing-guidelines.md
- harness-evals/harness-docs/olm-packaging-guidelines.md
- harness-evals/harness-docs/architecture/components.md
|
/lgtm |
|
/hold , please remove the hold once you are good with content review @arun717 |
|
Scheduling required tests: |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arun717, shivprakashmuley The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Provide AGENTS.md, ai-docs architecture/domain/ADRs, and deep-dive guideline playbooks so agents and contributors can follow operator-specific patterns without rediscovering them from code.
Move ai-docs and coding guidelines under harness-evals/harness-docs, add stage eval stubs, and keep AGENTS.md at the repo root so /opsx-constitute and OpenSpec eval gates can consume them.
Fix the error-handling §7 anchor and correct the verify-bindata checklist target so open review comments match the docs.
c1a7088 to
7558d44
Compare
|
New changes are detected. LGTM label has been removed. |
Document that make update-manifests also runs hack/update-istio-csr-manifests.sh alongside the cert-manager and trust-manager updaters.
|
@arun717: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
3 participants
Summary
AGENTS.mdat the repo root as the agent entrypoint for cert-manager-operator.harness-evals/layout:harness-evals/harness-docs/— architecture, domain CRDs, ADRs, development/testing guides, and deep-dive coding guidelines (controllers, OLM packaging, FIPS, security, testing, etc.) for/opsx-constituteharness-evals/evals/— stage eval stubs (repo-assessment,plan,tasks,code-generation) for OpenSpec quality gatesdocs/(proxy,cloud_credentials,operand_metrics).Test plan
AGENTS.mdintoharness-evals/harness-docs/pkg/operator/starter.goharness-evals/harness-docs/via/opsx-constituteand find stage eval files underharness-evals/evals/Summary by CodeRabbit