CM-875: Updates go.mod required for the e2e tests and improvements to Makefile #369
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references CM-875 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target 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. |
WalkthroughMakefile replaced with dynamic semver/image helpers, stricter shell/XDG behavior, and many new build/bundle/catalog/tool targets; kustomize manifests updated; Go module/tooling dependencies bumped; several hack scripts standardized to repo-root and vendor/local-bin usage; new unified tool downloader and govulncheck script added; dependencymagnet file removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/verify-types.sh (1)
19-25:⚠️ Potential issue | 🟠 MajorEnsure typelinter scans repository API packages, not just tools.
Line 19-25 runs from the tools directory with
go run -C tools ./typelinter ... ./.... The./...pattern is evaluated relative to tools, which only resolves to the typelinter package itself. This bypasses the API packages at the repository root that this script is intended to check. Pass the repository root explicitly to ensure API types are validated.🛠️ Proposed fix
+ROOT_DIR="$(cd "$(dirname "$0")/.." && pwd)" go run -C tools ./typelinter \ -whitelist="^(?:\[]|\*|map\[string])*(?:$builtins|(?:$pkgs)\.[A-Za-z0-9]+)\$" \ -excluded=github.com/openshift/api/build/v1.BuildStatus:Duration \ -excluded=github.com/openshift/api/image/dockerpre012.Config:ExposedPorts \ -excluded=github.com/openshift/api/image/dockerpre012.ImagePre012:Created \ -excluded=github.com/openshift/api/imageregistry/v1.ImagePrunerSpec:KeepYoungerThan \ - ./... + "${ROOT_DIR}"/...
🤖 Fix all issues with AI agents
In `@hack/update-clientgen.sh`:
- Line 11: The assignment to SCRIPT_ROOT uses an implicit array expansion of
BASH_SOURCE; change the expansion to use an explicit index so it reads
${BASH_SOURCE[0]} (i.e., update the SCRIPT_ROOT line that currently references
${BASH_SOURCE} to use ${BASH_SOURCE[0]}) to satisfy ShellCheck SC2128 and ensure
explicit, portable behavior.
In `@hack/update-protobuf.sh`:
- Around line 20-22: The script writes build artifacts to _output/bin but never
ensures that directory exists, causing builds to fail on clean checkouts; add a
step before the two go build commands to create the directory (e.g., run mkdir
-p _output/bin) so the targets _output/bin/go-to-protobuf and
_output/bin/protoc-gen-gogo can be written successfully.
In `@Makefile`:
- Around line 352-358: The bundle target currently invokes $(OPERATOR_SDK)
generate bundle without including $(BUNDLE_GEN_FLAGS), so flags like
USE_IMAGE_DIGESTS=true and --overwrite=false are ignored; update the bundle rule
to pass $(BUNDLE_GEN_FLAGS) into the generate bundle invocation (e.g., include
$(BUNDLE_GEN_FLAGS) before --version $(BUNDLE_VERSION) $(BUNDLE_METADATA_OPTS))
so USE_IMAGE_DIGESTS and other bundle generation flags are honored when running
the operator-sdk generate bundle command in the bundle target.
- Around line 38-42: The IMG_VERSION semver validation is unused because IMG is
hardcoded to :latest; either remove the IMG_VERSION validation or make IMG
derive from IMG_VERSION. To fix, update the IMG definition (the IMG variable) to
include IMG_VERSION (e.g., set IMG := your-registry/your-image:$(IMG_VERSION) or
append :$(IMG_VERSION) instead of :latest) so the validated version is actually
used, and keep BUNDLE_IMG and CATALOG_IMG behavior consistent; alternatively, if
you prefer always using latest, remove the IMG_VERSION validation block (the
validate-semver call and error) so there is no unused check.
- Around line 485-488: The Makefile uses the target $(LOCALBIN) but never
defines the LOCALBIN variable; add a default definition like LOCALBIN ?=
$(BIN_DIR) near the top (before the target definition of $(LOCALBIN)) so
$(LOCALBIN) expands correctly, and verify dependent rules that reference
$(LOCALBIN) (prerequisites listed after BIN_DIR usage) will now resolve
properly.
- Around line 472-482: The get-bin Makefile macro currently downloads binaries
without verifying integrity; update get-bin to accept a 4th parameter (expected
checksum or URL to checksum) and perform a checksum verification step after curl
and before chmod, failing the target if the checksum does not match (use
sha256sum or shasum/openssl as available); then update hack/helm.sh and
hack/operator-sdk.sh to pass the expected checksum (or checksum-file URL and
logic to fetch it) into get-bin and to abort on mismatch; ensure error messages
in get-bin reference the binary name and expected checksum and that the step
skips verification only when the 4th parameter is empty and that the download
and checksum commands are POSIX-safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@hack/download-tools.sh`:
- Around line 38-59: The verify_checksum function currently unconditionally
calls sha256sum, which breaks on macOS; modify verify_checksum to detect which
verifier is available (check with command -v sha256sum and fallback to command
-v shasum) and use sha256sum -c - when available or shasum -a 256 -c - as the
fallback, piping the same "hash filename" string into the chosen verifier;
ensure you capture and return the verifier's exit code and print a clear error
message via echo when the verifier is missing or the check fails so callers of
verify_checksum see a non-zero return on failure.
In `@Makefile`:
- Around line 250-260: The ginkgo label filter argument in the test-e2e Makefile
target is not quoted, so -ginkgo.label-filter=$(E2E_GINKGO_LABEL_FILTER) can be
split by the shell (e.g., spaces and &&); update the test-e2e recipe to pass the
label filter as a single argument by quoting the expansion (i.e., change the
-ginkgo.label-filter invocation used in the test-e2e target to use quotes around
$(E2E_GINKGO_LABEL_FILTER)) so the entire filter string is delivered to go test;
verify behavior via the test-e2e target that uses -ginkgo.label-filter.
🧹 Nitpick comments (1)
hack/update-cert-manager-manifests.sh (1)
19-31: Consider validating tool availability before use.The script assumes
./bin/yqand./bin/jsonnetexist without checking. If invoked directly (outside Makefile targets that provision these tools), the failure message will be unclear.A quick existence check at the start would improve usability:
for tool in ./bin/yq ./bin/jsonnet; do [[ -x "$tool" ]] || { echo "Error: $tool not found. Run 'make tools' first."; exit 1; } done
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hack/govulncheck.sh`:
- Around line 45-62: The script currently treats lack of "pkg.go.dev" in
RESULTS_FILE as a failure, causing false failures on clean scans; change the
govulncheck invocation to capture its exit code (use GOVULNCHECK_BIN and
RESULTS_FILE) instead of grepping output, then only treat unexpected exit codes
as errors—allow exit code 0 (clean) and 3 (vulnerabilities found) as valid
results and exit with error only if the captured rc is neither 0 nor 3; keep the
ARTIFACT_DIR copy logic and print RESULTS_FILE on real failure to aid debugging.
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
|
/test verify-deps |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hack/verify-deps.sh`:
- Around line 23-27: The duplicate-detection pipeline in hack/verify-deps.sh
currently filters out replace directives (the grep -v "=>") which hides version
conflicts between replaced and non-replaced entries; update the command that
computes the local variable duplicates (the pipeline that reads "$modules_txt"
and extracts package names into duplicates) to remove the grep -v "=>" filter so
replace lines are included in the awk '{print $2}' / sort | uniq -d check, and
adjust the surrounding comment to reflect that replace directives are now
considered when detecting duplicates.
🧹 Nitpick comments (2)
hack/download-tools.sh (1)
77-78: Consider shellcheck recommendations for robustness.The trap uses double quotes causing immediate variable expansion. While this works because
tempdirdoesn't change, it's fragile if refactored. Additionally, combining declaration and assignment can maskmktempfailures.♻️ Proposed fix (applies to similar patterns at lines 105-106 and 136-137)
- local tempdir=$(mktemp -d) - trap "rm -rf ${tempdir}" RETURN + local tempdir + tempdir=$(mktemp -d) + trap 'rm -rf ${tempdir}' RETURNMakefile (1)
478-491: Dead code:get-binmacro appears unused.The
get-binmacro is defined but all binary downloads usehack/download-tools.shinstead (lines 520-527). Consider removing it to reduce maintenance burden, or document if it's kept for external use.
|
/test e2e-operator-aws-proxy e2e-operator-aws-sts e2e-operator-gcp-ovn e2e-operator-gcp-workload-identity |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 245-248: The recipe in target "test" uses make's $(shell ...)
which is evaluated at parse time and breaks fresh builds; change it to perform
command substitution at recipe runtime by moving the command into the shell and
escaping the $ so Make doesn't evaluate it (e.g. replace
KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION)
--bin-dir $(ENVTEST_ASSETS_DIR) -p path)" with a runtime substitution like
KUBEBUILDER_ASSETS="$$( $(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir
$(ENVTEST_ASSETS_DIR) -p path )" in the "test" recipe so SETUP_ENVTEST
prerequisite can create the tool before the command runs).
Signed-off-by: Bharath B <bhb@redhat.com>
Signed-off-by: Bharath B <bhb@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 264-267: The three oc wait lines in the Makefile use "|| $(MAKE)
test-e2e-debug-cluster" which runs the debug target on failure but lets the
overall recipe continue if that debug target exits successfully; change each
fallback to run the debug target and then force a non-zero exit so the recipe
fails (e.g., invoke test-e2e-debug-cluster and then exit 1 or return false).
Update the lines referencing the oc wait commands for
deployment/cert-manager-cainjector, deployment/cert-manager, and
deployment/cert-manager-webhook so they call test-e2e-debug-cluster and then
explicitly fail the shell to prevent masking timeout failures.
| oc wait --for=condition=Available=true deployment/cert-manager-cainjector -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster | ||
| oc wait --for=condition=Available=true deployment/cert-manager -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster | ||
| oc wait --for=condition=Available=true deployment/cert-manager-webhook -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster | ||
| @echo "---- /Waiting for stable state ----" |
There was a problem hiding this comment.
Wait failures will not stop the recipe after debug output.
The || $(MAKE) test-e2e-debug-cluster pattern runs debug on failure, but if the debug target succeeds (exits 0), the recipe continues to the next wait command instead of failing. This could mask timeout failures and allow tests to run against an unstable cluster.
Proposed fix
.PHONY: test-e2e-wait-for-stable-state
test-e2e-wait-for-stable-state:
`@echo` "---- Waiting for stable state ----"
@# This ensures the test-e2e-debug-cluster is called if a timeout is reached.
- oc wait --for=condition=Available=true deployment/cert-manager-cainjector -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster
- oc wait --for=condition=Available=true deployment/cert-manager -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster
- oc wait --for=condition=Available=true deployment/cert-manager-webhook -n cert-manager --timeout=120s || $(MAKE) test-e2e-debug-cluster
+ oc wait --for=condition=Available=true deployment/cert-manager-cainjector -n cert-manager --timeout=120s || { $(MAKE) test-e2e-debug-cluster; exit 1; }
+ oc wait --for=condition=Available=true deployment/cert-manager -n cert-manager --timeout=120s || { $(MAKE) test-e2e-debug-cluster; exit 1; }
+ oc wait --for=condition=Available=true deployment/cert-manager-webhook -n cert-manager --timeout=120s || { $(MAKE) test-e2e-debug-cluster; exit 1; }
`@echo` "---- /Waiting for stable state ----"🤖 Prompt for AI Agents
In `@Makefile` around lines 264 - 267, The three oc wait lines in the Makefile use
"|| $(MAKE) test-e2e-debug-cluster" which runs the debug target on failure but
lets the overall recipe continue if that debug target exits successfully; change
each fallback to run the debug target and then force a non-zero exit so the
recipe fails (e.g., invoke test-e2e-debug-cluster and then exit 1 or return
false). Update the lines referencing the oc wait commands for
deployment/cert-manager-cainjector, deployment/cert-manager, and
deployment/cert-manager-webhook so they call test-e2e-debug-cluster and then
explicitly fail the shell to prevent masking timeout failures.
|
/test e2e-operator-aws-proxy e2e-operator-aws-sts e2e-operator-gcp-ovn e2e-operator-gcp-workload-identity |
|
@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. |
| .PHONY: verify | ||
| verify: verify-scripts verify-deps fmt vet ## Run all verification checks. | ||
|
|
||
| .PHONY: verify-scripts | ||
| verify-scripts: verify-bindata ## Run script-based verification checks. | ||
| hack/verify-deepcopy.sh | ||
| hack/verify-clientgen.sh | ||
| hack/verify-bundle.sh | ||
|
|
||
| .PHONY: verify-deps | ||
| verify-deps: ## Verify Go module dependencies are correct. | ||
| hack/verify-deps.sh |
There was a problem hiding this comment.
Didn't realize make verify would also run verify-deps. Perhaps we could consider removing the duplicated - as: verify-deps step in our presubmit CI later: https://github.com/openshift/release/blob/c68eff8b5f86d6bf11028d3a73eec91dec69aa88/ci-operator/config/openshift/cert-manager-operator/openshift-cert-manager-operator-master.yaml#L52-L56
There was a problem hiding this comment.
Yeah I too was wondering why twice, and I thought maybe to check any uncomitted changes which might pop up during verify. I think we can update the release config to remove verify-deps.
openshift-ci
bot
added
the
qe-approved
|
@bharath-b-rh: This pull request references CM-875 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, lunarwhite 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 |
3 participants
The main motivation for the PR is to add a new tool for validating APIs in
testdir and this required moving the go.mod fromtest/e2etotestto reuse the same for the test specific requirements. And this change caused revisiting the main go.mod and the improvements to the Makefile required an update to the tools go.mod.The PR contains below changes:
govulncheckkustomization.yamlin config dir to fix the warnings emitted duringmake bundle