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 3 commits into
Conversation
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.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arun717 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 |
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. |
WalkthroughChangesOperator guidance and documentation
Tool download hardening
Estimated code review effort: 2 (Simple) | ~10 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
| # Error Handling Guidelines | ||
|
|
||
| Scope: the controller-runtime (ctrl-runtime) stack — **IstioCSR** and **TrustManager**. CertManager | ||
| uses library-go and follows a different model (see [§7](#7-library-go-vs-ctrl-runtime-do-not-mix-patterns)). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the invalid §7 link fragment.
The link on Line 4 does not match the generated anchor for the heading on Line 140. Use the valid fragment or add an explicit anchor.
Proposed fix
-[§7](`#7-library-go-vs-ctrl-runtime-do-not-mix-patterns`)
+[§7](`#7-library-go-vs-ctrl-runtime--do-not-mix-patterns`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses library-go and follows a different model (see [§7](#7-library-go-vs-ctrl-runtime-do-not-mix-patterns)). | |
| uses library-go and follows a different model (see [§7](`#7-library-go-vs-ctrl-runtime--do-not-mix-patterns`)). |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 4-4: Link fragments should be valid
(MD051, link-fragments)
🤖 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/error-handling-guidelines.md` at 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.
Source: Linters/SAST tools
| - `replaces` must point at the exact previous CSV name shipped in the same channel (upgrade edge). | ||
| - `olm.skipRange` should cover all previously-published z-streams so upgrades from any older | ||
| installed version resolve correctly; bump the lower bound as old minors are dropped and the | ||
| upper bound to the new `<X.Y.0>` being released. | ||
| - Update **both** when cutting a new minor/z-stream; keep `bundle/metadata/annotations.yaml` | ||
| channel labels consistent with the release train. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Read the OLM packaging guidelines file to see exact text at lines 57-62 and 163-169
cat -n docs/olm-packaging-guidelines.md | sed -n '50,75p'Repository: openshift/cert-manager-operator
Length of output: 1948
🏁 Script executed:
# Also check lines 163-169
cat -n docs/olm-packaging-guidelines.md | sed -n '160,175p'Repository: openshift/cert-manager-operator
Length of output: 1948
🏁 Script executed:
# Look for actual CSV files in the repository to see real skipRange usage
find . -name "*.yaml" -o -name "*.yml" | grep -E "(csv|bundle|manifests)" | head -20Repository: openshift/cert-manager-operator
Length of output: 1948
🏁 Script executed:
# Search for skipRange usage in the repository
rg "skipRange" --type yaml -A 2 -B 2Repository: openshift/cert-manager-operator
Length of output: 1948
🌐 Web query:
OLM skipRange semver range semantics operator-sdk
💡 Result:
In Operator Lifecycle Manager (OLM), olm.skipRange is an annotation used within a ClusterServiceVersion (CSV) to define a range of semantic versions (semver) that can be skipped during an automatic operator upgrade [1][2][3]. When a channel head contains this annotation, if an installed operator's version falls within the specified range, OLM will automatically upgrade the operator to the latest entry in that channel, effectively skipping the intermediate versions [4][5][3]. Key technical details regarding olm.skipRange include: Semver Semantics: OLM utilizes the blang/semver Go library to parse and evaluate the range string [1][2]. The format must adhere to the semver range syntax supported by that library (e.g., ">=1.0.0 <1.0.3") [1][4][2]. Operational Behavior: - Precedence: When searching for updates, OLM evaluates update paths based on specific criteria. The channel head with a skipRange annotation is prioritized if the currently installed version falls within the defined range [4][5][3]. - Pruning: A significant consequence of using skipRange is that versions within the skipped range may be pruned from the update graph, making them unavailable for new installations via the spec.startingCSV field in a Subscription [1][2]. - Best Practice: To maintain installability of past versions while still leveraging skipRange, it is recommended to also define a replaces field in the CSV that points to the immediate previous version [1][2]. Operator SDK Integration: While the Operator SDK provides tooling to build and manage operators—including generating the manifests required by OLM—olm.skipRange is an OLM-specific feature [6]. The SDK facilitates the creation of the necessary CSVs and metadata where this annotation is applied, but the actual logic for evaluating the range and managing the upgrade graph is handled by the OLM controller (or the newer OLM v1/Operator Controller) in the cluster [7][8][6]. When transitioning to OLM v1 (Operator Controller), the behavior remains similar to legacy OLM for compatibility, though OLM v1 evaluates upgrade paths differently as it does not rely on a strict replaces chain [7][8]. OLM v1 identifies all entries that have replace, skip, or skipRange values covering the currently installed version to determine valid upgrade paths [7][8][9].
Citations:
- 1: https://v0-18-z.olm.operatorframework.io/docs/concepts/olm-architecture/operator-catalog/creating-an-update-graph/
- 2: https://olm.operatorframework.io/docs/concepts/olm-architecture/operator-catalog/creating-an-update-graph/
- 3: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/how-to-update-operators.md
- 4: Support semver ranges of versions to skip in the head of a channel operator-framework/operator-lifecycle-manager#834
- 5: https://docs.okd.io/4.22/operators/understanding/olm/olm-workflow.html
- 6: https://github.com/operator-framework/operator-sdk/blob/master/proposals/sdk-integration-with-olm.md
- 7: https://operator-framework.github.io/operator-controller/concepts/upgrade-support/
- 8: https://docs.okd.io/latest/extensions/ce/update-paths.html
- 9: https://docs.okd.io/4.22/extensions/ce/update-paths.html
🌐 Web query:
OLM olm.skipRange exclusive upper bound version comparison
💡 Result:
In Operator Lifecycle Manager (OLM), the olm.skipRange annotation uses the blang/semver Go library to define version ranges [1][2]. The version comparison for the exclusive upper bound follows standard semantic versioning (SemVer) rules [1][3]. When you specify an exclusive upper bound, such as <1.0.3, the range excludes any version that is equal to or greater than 1.0.3 [1][2]. For example, in the range >=1.0.0 <1.0.3, OLM considers any installed version that satisfies this range (e.g., 1.0.0, 1.0.1, 1.0.2) as eligible for an update to the bundle containing the skipRange annotation [1][3][2]. Key considerations regarding skipRange: - Inclusive/Exclusive semantics: The range follows standard SemVer logic, where < acts as an exclusive upper bound [1][3]. - Unbound ranges: It is recommended to avoid using unbound ranges (e.g., <1.0.3) if possible to prevent unintended update behavior [1][2]. - Catalog presence: Versions within the skipRange do not need to be present in the operator catalog for the bundle to be successfully added [1][2]. - Impact on installation: Using skipRange alone can prune previous versions from the update graph, making them unavailable for installation via a Subscription's startingCSV [1][2]. To maintain support for installing past versions while enabling skipRange, it is common practice to use skipRange in conjunction with a replaces field that connects to the immediate previous version in the update chain [1][2].
Citations:
- 1: https://v0-18-z.olm.operatorframework.io/docs/concepts/olm-architecture/operator-catalog/creating-an-update-graph/
- 2: https://olm.operatorframework.io/docs/concepts/olm-architecture/operator-catalog/creating-an-update-graph/
- 3: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/how-to-update-operators.md
Clarify the olm.skipRange upper bound for z-stream releases.
The documentation states to "bump the upper bound to the new <X.Y.0> being released" for both minor and z-stream releases. This rule is incorrect for z-stream releases. For a 1.20.1 release, using <1.20.0> excludes version 1.20.0, preventing users on that version from upgrading. Use the candidate version as the exclusive upper bound instead: <1.20.1> for a 1.20.1 z-stream release, and <1.21.0> for a 1.21.0 minor release. These bounds allow all previously-published versions in that minor series to upgrade correctly.
🤖 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/olm-packaging-guidelines.md` around lines 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.
| Adding a **new** operand image requires: a new `RELATED_IMAGE_*` env in | ||
| `config/manager/manager.yaml`, an entry in the `imageEnvMap` (or equivalent constants file) that | ||
| reads it, and re-running `make bundle` so the corresponding `relatedImages` entry is generated — | ||
| missing any of these will make `operator-sdk bundle validate` / disconnected mirroring fail. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target document ---'
cat -n docs/olm-packaging-guidelines.md | sed -n '70,190p'
printf '%s\n' '--- imageEnvMap and RELATED_IMAGE references ---'
rg -n -S 'imageEnvMap|RELATED_IMAGE_|relatedImages|bundle validate|make bundle' . \
-g '!vendor' -g '!node_modules' -g '!dist' -g '!build' | sed -n '1,240p'
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg '(^|/)(manager\.yaml|.*image.*map.*|.*constants.*|.*test.*|Makefile|bundle.*|config/.*)' | sed -n '1,240p'Repository: openshift/cert-manager-operator
Length of output: 1948
🏁 Script executed:
#!/bin/bash
set -eu
cat -n docs/olm-packaging-guidelines.md | sed -n '70,190p'
rg -n -S 'imageEnvMap|RELATED_IMAGE_|relatedImages|bundle validate|make bundle' . \
-g '!vendor' -g '!node_modules' -g '!dist' -g '!build' | sed -n '1,240p'Repository: openshift/cert-manager-operator
Length of output: 1948
🌐 Web query:
github openshift cert-manager-operator docs/olm-packaging-guidelines.md imageEnvMap RELATED_IMAGE_
💡 Result:
In the context of OpenShift and Operator Lifecycle Manager (OLM) packaging, the environment variable naming convention RELATED_IMAGE_ is a mandatory technical requirement used to identify container images associated with an operator [1][2]. This naming convention serves several critical functions in the OpenShift ecosystem: 1. Identification for Mirroring: It allows the OpenShift Build Service (OSBS) and other catalog-management tools to automatically discover all container images required by the operator [1]. This ensures these images can be correctly mirrored to the marketplace or private registries, which is essential for deployments in disconnected (air-gapped) or proxied environments [1][3][4]. 2. Automated Digest-Pinning: When an operator bundle is generated using tools like the Operator SDK (typically with the --use-image-digests flag), the build process scans for environment variables prefixed with RELATED_IMAGE_ [3][4][5]. It automatically resolves the image tags defined in these variables to their immutable content digests (SHA) and populates the spec.relatedImages section of the ClusterServiceVersion (CSV) [3][6][4]. 3. Configuration and Consistency: Using these variables ensures that the operand images used by the operator can be dynamically injected or overridden by the platform if necessary [1]. Developers are encouraged to use this prefix to ensure their operators remain compliant with Red Hat certification standards and function correctly across different OpenShift environments [3][2][5]. While spec.relatedImages in the CSV provides a static list of images for OLM catalog tools, the RELATED_IMAGE_* environment variables provide a runtime mechanism for the operator to know which image references to use, maintaining synchronization between the deployment configuration and the operator's runtime requirements [3][6][4]. Top results: [1][3][2][4][5]
Citations:
- 1: https://github.com/jsm84/rhm-env-example
- 2: Operator operand env var does not conform to OSBS naming requirements keycloak/keycloak#24017
- 3: bundle: nice UX around disconnected setup ocp-release-operator-sdk#113
- 4: https://github.com/opdev/simple-disconnected-operator
- 5: Helm operators generated by operator-sdk lack spec.relatedImages and thus fail RH certification operator-framework/operator-sdk#6482
- 6: How to sync
RELATED_IMAGEenvironment variables withspec.relatedImageswhen using image digests? operator-framework/operator-sdk#6923
🌐 Web query:
site:github.com/openshift/cert-manager-operator "Adding a new operand image requires"
💡 Result:
In the context of Kubernetes Operators, an operand is the application or software service that the operator manages [1][2]. Adding a new operand image generally involves configuring the operator to deploy the desired container image for the managed service [3]. While the specific implementation depends on the operator design, it typically requires the following steps: 1. Identify the Configuration Interface: Check the Custom Resource Definition (CRD) or configuration objects (e.g., a CertManager object) used by the operator to define its managed resources [1][4]. 2. Define the Operand Image: Operators are often configured to use specific images via environment variables, custom resource fields, or override configurations [3][5][6]. For example, in some implementations, related images are defined as environment variables passed to the operator container, or they can be overridden via the operator's configuration spec [3][4]. 3. Apply the Configuration: Use the Kubernetes API or command-line tools (such as oc or kubectl) to update the relevant Custom Resource (CR) with the new image reference [1][4]. This often involves updating a spec section that controls deployment configurations, such as environment variables, resource requirements, or custom arguments [7][4][6]. If you are working with a specific operator, consult its official documentation or the repository's README, as many operators require you to build the operator with specific image references or modify specific fields within the operator's managed CR to point to the desired image [1][2][4].
Citations:
- 1: https://github.com/openshift/cert-manager-operator
- 2: https://github.com/openshift/cert-manager-operator/blob/ae53ee1a5c3a515497f70435e9aaedba0a118f0d/README.md
- 3: 072db14
- 4: https://github.com/openshift/cert-manager-operator/blob/master/README.md
- 5: c49fd4e
- 6: 7d53547
- 7: [cert-manager-1.15] CM-411: Add support of NodeSelector and Tolerations to CertManager CRD for configuring the operand pods #214
🌐 Web query:
site:github.com/openshift/cert-manager-operator/blob "operator-sdk bundle validate" "imageEnvMap"
💡 Result:
There is no evidence in the provided search results to support the existence of an imageEnvMap option or feature within the operator-sdk bundle validate command. The search results discuss the configuration of the cert-manager-operator, including its Dockerfile, Makefile, and Go modules, but do not contain references to imageEnvMap. The operator-sdk bundle validate command is used to verify operator bundles against OLM standards, and users should typically consult the official Operator SDK documentation for a comprehensive list of available command flags and validation criteria.
Citations:
- 1: https://github.com/openshift/cert-manager-operator/blob/master/README.md
- 2: https://github.com/openshift/cert-manager-operator/blob/ac9cc761499788acf145f83ebeb2a7ee80dfa6a4/PROJECT
- 3: https://github.com/openshift/cert-manager-operator/blob/ae53ee1a5c3a515497f70435e9aaedba0a118f0d/README.md
- 4: https://github.com/openshift/cert-manager-operator/blob/master/go.mod
- 5: https://github.com/openshift/cert-manager-operator/blob/78de13d53779ba7b3a097978557f019bb1a9063a/Dockerfile
🌐 Web query:
https://raw.githubusercontent.com/openshift/cert-manager-operator/master/docs/olm-packaging-guidelines.md
💡 Result:
The document olm-packaging-guidelines.md, referenced in the context of the cert-manager-operator repository, refers to the standardized procedures and requirements for packaging an Operator for use with Red Hat's Operator Lifecycle Manager (OLM) [1][2]. In the context of the OpenShift and Operator Framework ecosystems, OLM packaging refers to the bundle format—a standardized way to package Operator metadata, Kubernetes manifests, and CRDs into a container image that OLM can use to install, manage, and upgrade software on a cluster [3][4][5]. Key components and practices associated with OLM packaging include: 1. Bundle Format: An Operator bundle is a container image containing the metadata and manifests (e.g., ClusterServiceVersion (CSV), CRDs, and roles) for a specific version of an Operator [1][6][4]. 2. ClusterServiceVersion (CSV): This is the primary entry point for OLM packaging, describing the Operator, its dependencies, install strategies, and runtime requirements [6][7]. 3. Catalog Images: Bundles are organized into catalogs, which are stored as container images and queried by OLM to provide available Operators to the cluster via OperatorHub [1][3][5]. 4. Tooling: The opm CLI tool is the standard utility provided by the Operator Framework for building, managing, and maintaining these catalogs and bundles [3][4][8]. 5. Guidelines and Compliance: When packaging Operators for OpenShift, authors must often comply with specific metadata requirements (such as OCP version annotations) and validation criteria to ensure compatibility and successful distribution via community or product catalogs [9]. For official technical guidance, developers typically refer to the Operator Framework's OLM documentation or the specific OpenShift documentation regarding operator packaging and development [3][7][8].
Citations:
- 1: https://github.com/cert-manager/cert-manager-olm
- 2: https://github.com/cert-manager/cert-manager-olm/blob/master/README.md
- 3: https://docs.okd.io/latest/operators/understanding/olm-packaging-format.html
- 4: https://docs.okd.io/4.9/operators/understanding/olm-packaging-format.html
- 5: https://docs.redhat.com/en/documentation/openshift_container_platform/4.12/html-sinble/operators/administrator-tasks
- 6: https://operator-framework.github.io/olm-book/docs/packaging-an-operator.html
- 7: https://olm.operatorframework.io/docs/
- 8: https://docs.redhat.com/en/documentation/openshift_container_platform/4.14/pdf/operators/developing-operators
- 9: https://redhat-openshift-ecosystem.github.io/operator-pipelines/users/packaging-required-criteria-ocp/
🌐 Web query:
Operator SDK bundle validate validates relatedImages CSV environment variables runtime image map
💡 Result:
The Operator SDK bundle validate command ensures that your bundle adheres to specific requirements, including the spec.relatedImages field in your ClusterServiceVersion (CSV) [1][2]. This field is used to list images required by the operator, which must be referenced by digest (SHA) to support disconnected environments [3][4]. Key aspects of how relatedImages and environment variables interact include: Related Images Discovery: The Operator SDK can automatically discover RELATED_IMAGE_ environment variables defined in your manager.yaml (or other container environments) and add them to the spec.relatedImages list in your generated CSV [5]. This is a recommended practice to ensure that your operator correctly advertises its required images to the Operator Lifecycle Manager (OLM) [5]. Environment Variables vs. Static Definitions: While you can define related images statically in your base CSV, it is highly recommended to use RELATED_IMAGE_ environment variables [5][6]. Using these variables ensures that when you build your bundle (specifically when using the --use-image-digests flag), the generated CSV is correctly populated with the image digests derived from your environment [7][3][4]. Validating with bundle validate: The operator-sdk bundle validate command checks the integrity and format of your bundle [2]. While it does not automatically "map" environment variables at runtime, it validates that your CSV meets the criteria for catalog submission [1]. To ensure your images are properly validated as digests, you should run make bundle with USE_IMAGE_DIGESTS=true [3][4]. This flag instructs the SDK to resolve images to their SHA digests and populate spec.relatedImages accordingly [3][4]. If you encounter certification or validation errors (e.g., missing spec.relatedImages), ensure that you have enabled digest resolution in your Makefile via BUNDLE_GEN_FLAGS or the USE_IMAGE_DIGESTS variable [4][8]. Note that spec.relatedImages is used primarily during catalog creation and for disconnected environment support; it does not play a direct role in cluster runtime image mapping, which relies on the RELATED_IMAGE_ environment variables themselves being correctly injected into your operator's deployment [5][9][8].
Citations:
- 1: https://master.sdk.operatorframework.io/docs/olm-integration/generation/
- 2: https://sdk.operatorframework.io/docs/cli/operator-sdk_bundle_validate/
- 3: https://docs.okd.io/4.17/operators/operator_sdk/osdk-generating-csvs.html
- 4: https://docs.okd.io/4.18/operators/operator_sdk/osdk-generating-csvs.html
- 5: Don't override relatedImages in config CSV. operator-framework/operator-sdk#5763
- 6: Don't override relatedImages in base CSV. #5763 operator-framework/operator-sdk#5765
- 7: How to sync
RELATED_IMAGEenvironment variables withspec.relatedImageswhen using image digests? operator-framework/operator-sdk#6923 - 8: Helm operators generated by operator-sdk lack spec.relatedImages and thus fail RH certification operator-framework/operator-sdk#6482
- 9:
generate bundle: explicitly support.spec.relatedImagesand.spec.skipsoperator-framework/operator-sdk#4884
Separate bundle metadata validation from runtime image-map validation.
operator-sdk bundle validate checks generated bundle metadata. It does not inspect the Go imageEnvMap or constants that consume each RELATED_IMAGE_* variable. Require both bundle validation and source-level or unit-test validation for every mapping. A missing runtime mapping can pass bundle validation and still break disconnected operand image resolution.
🤖 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/olm-packaging-guidelines.md` around lines 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.
| ## Verify scripts | ||
|
|
||
| | Script | Checks | Wired into `make verify-scripts`? | | ||
| |---|---|---| | ||
| | `hack/verify-bundle.sh` | Runs `make bundle` and fails if the tree has uncommitted diffs. | Yes | | ||
| | `hack/verify-deepcopy.sh` | Generated `zz_generated.deepcopy.go` is current. | Yes | | ||
| | `hack/verify-clientgen.sh` | Generated clientset/informers/listers/applyconfigurations are current. | Yes | | ||
| | `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` | CRD manifests match `config/crd/bases` and upgrades are compatible. | No — run directly | | ||
|
|
||
| Run `make verify` (or `make verify-scripts`) locally before sending a PR that touches | ||
| `api/`, `config/`, or `bindata/`; also run `hack/verify-crds*.sh` directly if operand CRDs changed, | ||
| since they are not invoked by any Makefile target in this repo. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align bindata verification instructions.
Line 154 says bindata verification runs through make verify-scripts, but the verification table does not list a bindata verifier. Line 130 names a separate make verify-bindata target. Add the bindata verifier to the table, or instruct authors to run make verify-bindata directly.
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 `@docs/olm-packaging-guidelines.md` around lines 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.
| make test-unit # go test ./... (excludes test/e2e, test/apis, test/utils) | ||
| make test-apis # envtest + ginkgo against api/operator/v1alpha1/tests fixtures | ||
| make test-e2e # ginkgo e2e suite against a live cluster (requires oc login) | ||
| make test-e2e-debug-cluster # dump operator/operand pod status, CSVs, CRDs on failure |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the test-unit exclusion rule or document its actual behavior.
The supplied Makefile uses grep -vE 'test/[e2e|apis|utils]'. The square brackets define a character class, not directory alternatives. This can exclude unrelated paths such as test/integration and fail to exclude test/helpers.
Use grep -vE 'test/(e2e|apis|utils)(/|$)' if these are the intended directories. Then keep this guide consistent with the Makefile.
Also applies to: 48-48
🤖 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/testing-guidelines.md` around lines 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.
| ```go | ||
| //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate | ||
| //counterfeiter:generate -o fakes . CtrlClient | ||
| type CtrlClient interface { Get, List, StatusUpdate, Update, UpdateWithRetry, Create, Delete, Patch, Exists } | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Read the testing-guidelines.md file around lines 81-85
sed -n '75,90p' docs/testing-guidelines.mdRepository: openshift/cert-manager-operator
Length of output: 1948
Use valid Go syntax or mark this block as pseudocode.
The interface definition type CtrlClient interface { Get, List, StatusUpdate, ... } is not valid Go syntax. A Go interface requires method signatures with parameter and return types. Replace the abbreviated method list with actual method signatures, or if this is pseudocode, use a non-Go code fence or move it into prose.
🤖 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/testing-guidelines.md` around lines 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.
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.
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.
|
|
||
| ## Decision | ||
|
|
||
| Keep **library-go** for CertManager (always-on). Use **controller-runtime** for IstioCSR and TrustManager, started only when `--unsupported-addon-features` enables the corresponding gates. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Describe resolved gate state, not only explicit flag opt-in.
This sentence says the controllers start only when --unsupported-addon-features enables their gates. IstioCSR is documented as GA with a default of true in adr-0003-feature-gates.md. State that controllers start when the resolved gate is enabled, including the default for IstioCSR.
🤖 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/decisions/adr-0001-dual-controller-frameworks.md`
at 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.
| | `hack/verify-bundle.sh` | Runs `make bundle` and fails if the tree has uncommitted diffs. | Yes | | ||
| | `hack/verify-deepcopy.sh` | Generated `zz_generated.deepcopy.go` is current. | Yes | | ||
| | `hack/verify-clientgen.sh` | Generated clientset/informers/listers/applyconfigurations are current. | Yes | | ||
| | `hack/verify-crds.sh` / `verify-crds-version-upgrade.sh` | CRD manifests match `config/crd/bases` and upgrades are compatible. | No — run directly | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the hack/ prefix for the CRD upgrade verifier.
Line [147] uses hack/verify-crds*.sh, but Lines [144] and [160] omit hack/ for verify-crds-version-upgrade.sh. From the repository root, use hack/verify-crds-version-upgrade.sh consistently.
Also applies to: 160-161
🤖 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` at 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.
| 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: 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. |
|
@arun717 please move contents of |
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
Documentation
Tests
Chores