CM-778: Bumps cert-manager, istio-csr and consolidates yq tool dependency#386
Open
bharath-b-rh wants to merge 4 commits intoopenshift:masterfrom
Open
CM-778: Bumps cert-manager, istio-csr and consolidates yq tool dependency#386bharath-b-rh wants to merge 4 commits intoopenshift:masterfrom
bharath-b-rh wants to merge 4 commits intoopenshift:masterfrom
Conversation
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references CM-778 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 epic to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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. |
WalkthroughBumped cert-manager from v1.19.2 → v1.19.4 and istio-csr from v0.15.0 → v0.16.0 across manifests, embedded assets, go.mod, Dockerfile, and tests. Added in-repo Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
hack/update-cert-manager-manifests.sh (1)
49-50: Harden temp-file cleanup for failure paths.Current cleanup is success-path only and can be brittle with
set -eif any file is already absent. Considertrap+rm -f.Proposed hardening patch
set -e + +cleanup() { + rm -f _output/manifest.yaml _output/manifest_as_array.json _output/targets_as_map.json +} +trap cleanup EXIT @@ -# cleanup temp files -rm _output/manifest.yaml _output/manifest_as_array.json _output/targets_as_map.json +# cleanup handled by trap🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/update-cert-manager-manifests.sh` around lines 49 - 50, Replace the brittle direct rm call with a robust cleanup function and trap: create a cleanup() function that does rm -f on _output/manifest.yaml _output/manifest_as_array.json _output/targets_as_map.json and register it with trap cleanup EXIT (and optionally ERR) so cleanup always runs even on failures; update the existing direct rm invocation to call that cleanup() or remove it to avoid set -e causing script termination when files are missing.hack/update-trust-manager-manifests.sh (1)
50-50: Consider using${MANIFESTS_PATH}for consistency.The cleanup command hardcodes
_output/manifestswhile the rest of the script uses${MANIFESTS_PATH}. This inconsistency could cause issues if the variable is changed later.♻️ Suggested fix
-rm -rf _output/manifests +rm -rf ${MANIFESTS_PATH}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/update-trust-manager-manifests.sh` at line 50, Replace the hardcoded cleanup target with the MANIFESTS_PATH variable: change the command that currently uses "rm -rf _output/manifests" to remove "${MANIFESTS_PATH}" instead so it is consistent with the rest of the script and respects any future changes to MANIFESTS_PATH (ensure the variable is defined before use).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/verify-crds-version-upgrade.sh`:
- Around line 60-61: The script uses the old yq v3 syntax (`./bin/yq r $f
apiVersion`) which is incompatible with yq v4; update the two invocations that
set v1CRDName and that read apiVersion to use yq v4's eval form (e.g. use yq e
'.apiVersion' and yq e '.metadata.name' on the file variable $f), ensuring you
pass the filename as an argument (e.g. yq e '<path>' "$f") and preserve
raw/string output behavior so the apiVersion comparison and v1CRDName assignment
continue to work.
- Around line 46-47: The script uses yq v3 syntax (`./bin/yq r $f apiVersion`
and `./bin/yq r $f metadata.name`) which fails with the repo's yq v4; update the
invocations to use yq v4 eval syntax by replacing the `yq r` calls with
`./bin/yq e '.apiVersion' $f` and `./bin/yq e '.metadata.name' $f` respectively
(these affect the lines that set v1beta1CRDName and similar checks); apply the
same replacements in the sibling script that contains the same yq usage (the one
referenced as verify-crds.sh) so all `yq r $f ...` occurrences become `yq e
'<path>' $f`.
In `@hack/verify-crds.sh`:
- Around line 6-7: Replace deprecated yq v3 invocations using "yq r" with yq v4
syntax "yq e" and the corresponding expression form: change the checks that call
"./bin/yq r $f apiVersion" to use yq e with the expression for apiVersion, and
change "./bin/yq r $f
spec.validation.openAPIV3Schema.properties.metadata.description" to use yq e
with the dotted-path expression for that field; do the same replacements for the
analogous occurrences in the verify-crds-version-upgrade.sh script (the checks
that read apiVersion and
spec.validation.openAPIV3Schema.properties.metadata.description) so the commands
work with yq v4.52.4.
---
Nitpick comments:
In `@hack/update-cert-manager-manifests.sh`:
- Around line 49-50: Replace the brittle direct rm call with a robust cleanup
function and trap: create a cleanup() function that does rm -f on
_output/manifest.yaml _output/manifest_as_array.json _output/targets_as_map.json
and register it with trap cleanup EXIT (and optionally ERR) so cleanup always
runs even on failures; update the existing direct rm invocation to call that
cleanup() or remove it to avoid set -e causing script termination when files are
missing.
In `@hack/update-trust-manager-manifests.sh`:
- Line 50: Replace the hardcoded cleanup target with the MANIFESTS_PATH
variable: change the command that currently uses "rm -rf _output/manifests" to
remove "${MANIFESTS_PATH}" instead so it is consistent with the rest of the
script and respects any future changes to MANIFESTS_PATH (ensure the variable is
defined before use).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c37868b1-2e18-4271-b9ab-df658f81b364
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumtest/go.sumis excluded by!**/*.sumvendor/github.com/cert-manager/cert-manager/LICENSESis excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (80)
Makefilebindata/cert-manager-deployment/cainjector/cert-manager-cainjector-cr.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-crb.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-deployment.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-leaderelection-rb.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-leaderelection-role.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-sa.yamlbindata/cert-manager-deployment/cainjector/cert-manager-cainjector-svc.yamlbindata/cert-manager-deployment/cert-manager/cert-manager-controller-approve-cert-manager-io-cr.yamlbindata/cert-manager-deployment/cert-manager/cert-manager-controller-approve-cert-manager-io-crb.yamlbindata/cert-manager-deployment/cert-manager/cert-manager-controller-certificatesigningrequests-cr.yamlbindata/cert-manager-deployment/cert-manager/cert-manager-controller-certificatesigningrequests-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-cluster-view-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-certificates-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-certificates-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-challenges-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-challenges-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-clusterissuers-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-clusterissuers-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-ingress-shim-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-ingress-shim-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-issuers-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-issuers-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-orders-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-controller-orders-crb.yamlbindata/cert-manager-deployment/controller/cert-manager-deployment.yamlbindata/cert-manager-deployment/controller/cert-manager-edit-cr.yamlbindata/cert-manager-deployment/controller/cert-manager-leaderelection-rb.yamlbindata/cert-manager-deployment/controller/cert-manager-leaderelection-role.yamlbindata/cert-manager-deployment/controller/cert-manager-sa.yamlbindata/cert-manager-deployment/controller/cert-manager-svc.yamlbindata/cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yamlbindata/cert-manager-deployment/controller/cert-manager-tokenrequest-role.yamlbindata/cert-manager-deployment/controller/cert-manager-view-cr.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-deployment.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-dynamic-serving-rb.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-dynamic-serving-role.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-mutatingwebhookconfiguration.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-sa.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-subjectaccessreviews-cr.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-subjectaccessreviews-crb.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-svc.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-validatingwebhookconfiguration.yamlbindata/istio-csr/cert-manager-istio-csr-clusterrole.yamlbindata/istio-csr/cert-manager-istio-csr-clusterrolebinding.yamlbindata/istio-csr/cert-manager-istio-csr-deployment.yamlbindata/istio-csr/cert-manager-istio-csr-leases-role.yamlbindata/istio-csr/cert-manager-istio-csr-leases-rolebinding.yamlbindata/istio-csr/cert-manager-istio-csr-metrics-service.yamlbindata/istio-csr/cert-manager-istio-csr-role.yamlbindata/istio-csr/cert-manager-istio-csr-rolebinding.yamlbindata/istio-csr/cert-manager-istio-csr-service.yamlbindata/istio-csr/cert-manager-istio-csr-serviceaccount.yamlbindata/istio-csr/istiod-certificate.yamlbundle/manifests/acme.cert-manager.io_challenges.yamlbundle/manifests/acme.cert-manager.io_orders.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/cert-manager.io_certificaterequests.yamlbundle/manifests/cert-manager.io_certificates.yamlbundle/manifests/cert-manager.io_clusterissuers.yamlbundle/manifests/cert-manager.io_issuers.yamlconfig/crd/bases/certificaterequests.cert-manager.io-crd.yamlconfig/crd/bases/certificates.cert-manager.io-crd.yamlconfig/crd/bases/challenges.acme.cert-manager.io-crd.yamlconfig/crd/bases/clusterissuers.cert-manager.io-crd.yamlconfig/crd/bases/issuers.cert-manager.io-crd.yamlconfig/crd/bases/orders.acme.cert-manager.io-crd.yamlconfig/manager/manager.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlgo.modhack/lib/yq.shhack/update-cert-manager-manifests.shhack/update-istio-csr-manifests.shhack/update-trust-manager-manifests.shhack/verify-crds-version-upgrade.shhack/verify-crds.shimages/ci/operand.Dockerfilepkg/controller/deployment/deployment_overrides_test.gopkg/operator/assets/bindata.gotest/go.mod
💤 Files with no reviewable changes (1)
- hack/lib/yq.sh
bharath-b-rh
force-pushed
the
rebase-upstream
branch
from
f786c8a to
f4b079d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hack/verify-crds.sh (1)
8-8:⚠️ Potential issue | 🟠 MajorUse yq v4 path expressions with leading
.for these field reads.At Line 8 and Line 13,
yq eis called with'spec....'instead of'.spec....'. With yq v4, this can mis-evaluate or error and invalidate the checks.Suggested fix
- if [[ $(./bin/yq e 'spec.validation.openAPIV3Schema.properties.metadata.description' $f) != "null" ]]; then + if [[ "$(./bin/yq e '.spec.validation.openAPIV3Schema.properties.metadata.description' "$f")" != "null" ]]; then @@ - if [[ $(./bin/yq e 'spec.preserveUnknownFields' $f) != "false" ]]; then + if [[ "$(./bin/yq e '.spec.preserveUnknownFields' "$f")" != "false" ]]; then#!/bin/bash # Verify no yq eval expressions remain that start with bare 'spec.' in this script. rg -nP "\./bin/yq\s+e\s+'spec\." hack/verify-crds.shExpected result: no matches.
Also applies to: 13-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/verify-crds.sh` at line 8, Change the yq v4 path expressions to use a leading dot so they evaluate correctly: update the invocations that call ./bin/yq e 'spec.validation.openAPIV3Schema.properties.metadata.description' and ./bin/yq e 'spec.validation.openAPIV3Schema.properties.status.description' to use './bin/yq e \'.spec.validation.openAPIV3Schema.properties.metadata.description\'' and './bin/yq e \'.spec.validation.openAPIV3Schema.properties.status.description\'' respectively (i.e., prefix the path with a dot in both yq expressions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/update-istio-csr-manifests.sh`:
- Around line 6-9: The cleanup function uses rm without -f which errors if
_output/istio-csr-manifest.yaml doesn't exist; update the cleanup() function to
call rm -f _output/istio-csr-manifest.yaml so the removal is idempotent and
matches the behavior in hack/update-cert-manager-manifests.sh; ensure you only
change the rm invocation in cleanup() (refer to the cleanup function and the
filename _output/istio-csr-manifest.yaml).
In `@hack/verify-crds.sh`:
- Around line 4-8: The script uses unsafe command substitution and unquoted
filename variables plus incorrect yq v4 paths: change the for-loop to a safe
find-read loop (use while IFS= read -r -d '' f ... or similar) and ensure every
use of the filename variable ($f) is quoted (e.g., "$f"); additionally update yq
expressions to include the leading dot (use
'.spec.validation.openAPIV3Schema.properties.metadata.description' and
'.spec.preserveUnknownFields') where used (references: variable f,
v1beta1CRDName and the yq invocations in the loop).
---
Duplicate comments:
In `@hack/verify-crds.sh`:
- Line 8: Change the yq v4 path expressions to use a leading dot so they
evaluate correctly: update the invocations that call ./bin/yq e
'spec.validation.openAPIV3Schema.properties.metadata.description' and ./bin/yq e
'spec.validation.openAPIV3Schema.properties.status.description' to use './bin/yq
e \'.spec.validation.openAPIV3Schema.properties.metadata.description\'' and
'./bin/yq e \'.spec.validation.openAPIV3Schema.properties.status.description\''
respectively (i.e., prefix the path with a dot in both yq expressions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 439e4930-40d1-4dbc-a252-84dcab61122d
📒 Files selected for processing (6)
hack/lib/yq.shhack/update-cert-manager-manifests.shhack/update-istio-csr-manifests.shhack/update-trust-manager-manifests.shhack/verify-crds-version-upgrade.shhack/verify-crds.sh
💤 Files with no reviewable changes (1)
- hack/lib/yq.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/verify-crds-version-upgrade.sh
- hack/update-cert-manager-manifests.sh
…ript artifacts Signed-off-by: Bharath B <bhb@redhat.com>
bharath-b-rh
force-pushed
the
rebase-upstream
branch
from
f4b079d to
35dcf70
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/verify-crds.sh`:
- Around line 6-16: The script checks for apiVersion "apiextensions.k8s.io/v1"
but then reads v1beta1 schema paths and sets an unused v1beta1CRDName variable,
so the schema validations are ineffective; either change the apiVersion test to
"apiextensions.k8s.io/v1beta1" if you intend to validate v1beta1 CRDs, or
(recommended for v1) keep the v1 test and update the yq paths to iterate
.spec.versions[] and inspect each version's .schema.openAPIV3Schema for
metadata.description and other rules, and remove or rename the misleading
v1beta1CRDName variable; also ensure the .spec.preserveUnknownFields check
remains at the spec level for v1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97384bd6-a618-4c47-8cb2-a9071455ccd0
📒 Files selected for processing (6)
hack/lib/yq.shhack/update-cert-manager-manifests.shhack/update-istio-csr-manifests.shhack/update-trust-manager-manifests.shhack/verify-crds-version-upgrade.shhack/verify-crds.sh
💤 Files with no reviewable changes (1)
- hack/lib/yq.sh
✅ Files skipped from review due to trivial changes (1)
- hack/update-istio-csr-manifests.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/update-cert-manager-manifests.sh
- hack/verify-crds-version-upgrade.sh
Contributor
Author
|
/test e2e-operator-tech-preview |
Contributor
|
@bharath-b-rh: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR has following changes