CNFCERT-1258: Add kustomize validation check#2398
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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 |
|
https://github.com/openshift-kni/numaresources-operator/actions/runs/18952979705/job/54122262347?pr=2398 It's already running and passing! 🎉 |
|
The CI failure is legit, PTAL. |
sebrandon1
force-pushed
the
add_kustomization_test
branch
from
04ddb36 to
589fcdc
Compare
|
Not sure how beneficial is this validation, but should not hurt either |
sebrandon1
force-pushed
the
add_kustomization_test
branch
from
589fcdc to
3a6f47c
Compare
sebrandon1
force-pushed
the
add_kustomization_test
branch
from
3a6f47c to
76b978e
Compare
sebrandon1
changed the title
|
@sebrandon1: This pull request references CNFCERT-1258 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 "4.22.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. |
|
/retest |
|
still relevant? |
sebrandon1
force-pushed
the
add_kustomization_test
branch
from
8826c5f to
2666389
Compare
openshift-ci
Bot
added
the
needs-rebase
📝 WalkthroughWalkthroughThis pull request updates the NUMA Resources Operator project from version 4.21 to 4.22 across the entire infrastructure. Changes include updating container image tags, version references, and related build configuration files. A new GitHub Actions workflow for Kustomize validation and corresponding test script were also added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
PR needs rebase. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.tests.md (1)
33-37:⚠️ Potential issue | 🟠 MajorFix broken shell continuation in runnable examples.
The command block is not copy/paste runnable because Line 35 misses a trailing
\before Line 36. The same issue recurs later before--skip(Line 114). Please fix all affectedpodman runsnippets.Proposed doc fix
podman run -ti \ -v $KUBECONFIG:/kubeconfig:z \ - -e KUBECONFIG=/kubeconfig + -e KUBECONFIG=/kubeconfig \ -e E2E_NROP_INSTALL_SKIP_KC=true \ quay.io/openshift-kni/numaresources-operator-tests:4.22.999-snapshot @@ -e E2E_NUMACELL_DEVICE_PLUGIN_URL=${E2E_IMAGE_URL} \ -e E2E_PAUSE_IMAGE_URL=${E2E_IMAGE_URL} \ - ${E2E_IMAGE_URL} + ${E2E_IMAGE_URL} \ --skip '.*reboot_required.*'As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.tests.md` around lines 33 - 37, The shell continuation in the podman run snippets is broken: ensure each line intended to continue the command ends with a trailing backslash so the entire podman run invocation is copy/paste runnable; specifically add a trailing "\" after the KUBECONFIG=/kubeconfig line in the podman run that sets -v $KUBECONFIG and -e KUBECONFIG, and likewise fix the later podman run snippet that wraps the --skip argument (look for the podman run lines and the --skip flag) so every fragmented line ends with "\" to maintain proper shell continuation.
🧹 Nitpick comments (3)
.github/workflows/release-testsuite.yaml (1)
26-26: Avoid hardcoding the release tag directly in Line 26.This value changes every release and is easy to miss; centralizing it as a workflow env var reduces drift risk.
♻️ Proposed refactor
name: CI testsuite release flow +env: + TEST_IMAGE_TAG: 4.22.999-snapshot on: workflow_dispatch: @@ - name: Build Image id: build-image uses: redhat-actions/buildah-build@v2 with: image: numaresources-operator-tests - tags: 4.22.999-snapshot + tags: ${{ env.TEST_IMAGE_TAG }} dockerfiles: | ./Dockerfile.testsAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-testsuite.yaml at line 26, The hardcoded tag value "4.22.999-snapshot" should be replaced with a reusable env var; define RELEASE_TAG in the workflow's top-level env block (or as a workflow_dispatch/input) and change the tags entry to reference it (use GitHub Actions expression syntax like ${{ env.RELEASE_TAG }}), so update the workflow file to declare RELEASE_TAG and replace the literal "4.22.999-snapshot" with that env reference..github/workflows/kustomize-validation.yml (1)
19-43: Consider pinning kustomize version for reproducible builds.The current approach downloads the latest
install_kustomize.shscript from themasterbranch, which installs the latest kustomize version. This could lead to non-reproducible CI behavior if a new kustomize version introduces breaking changes or incompatibilities.Consider specifying a version by passing an argument to the install script (e.g.,
| bash -s -- <version>) or using a pinned release URL instead ofmaster.💡 Suggested improvement for version pinning
while [ $ATTEMPT -le $MAX_ATTEMPTS ]; do echo "Attempt $ATTEMPT of $MAX_ATTEMPTS: Downloading kustomize install script..." - if curl -fsSL --retry 3 --retry-delay 2 "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash; then + if curl -fsSL --retry 3 --retry-delay 2 "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 5.5.0; then echo "Successfully downloaded and installed kustomize" break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/kustomize-validation.yml around lines 19 - 43, The workflow currently pipes the installer from the master branch (the curl line fetching "install_kustomize.sh") which installs the latest kustomize and causes non-deterministic CI; change the curl invocation to pin a specific kustomize version by either passing the version to the script (invoke the script with "bash -s -- <VERSION>" instead of plain "bash") or by downloading a release-tagged install script URL (replace the master URL with the specific release tag) so that the installed kustomize version is deterministic; update any references around MAX_ATTEMPTS/ATTEMPT and the sudo mv/kustomize steps to remain compatible with the pinned installer behavior.hack/test-kustomize.sh (1)
38-45: Make exclusion matching normalized and prefix-aware.Current equality matching is fragile (
./prefix differences) and only skips exact directories, not nested overlays under an excluded root.♻️ Proposed refactor
is_excluded() { - local dir="$1" + local dir="${1#./}" for excluded in "${EXCLUDED_DIRS[@]}"; do - if [ "$dir" = "$excluded" ]; then + local ex="${excluded#./}" + if [[ "$dir" == "$ex" || "$dir" == "$ex/"* ]]; then return 0 fi done return 1 }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 64-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/test-kustomize.sh` around lines 38 - 45, The is_excluded function currently uses exact equality and fails on ./ prefixes or nested paths; update is_excluded to normalize the incoming dir and each EXCLUDED_DIRS entry by removing any leading ./ and trailing slashes, then test exclusion by checking either exact equality or prefix-match (excluded + "/" as a prefix) so directories under an excluded root are also skipped; reference the is_excluded function and the EXCLUDED_DIRS array in your change and ensure the same normalization/prefix logic is applied wherever is_excluded is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.tests.md`:
- Around line 33-37: The shell continuation in the podman run snippets is
broken: ensure each line intended to continue the command ends with a trailing
backslash so the entire podman run invocation is copy/paste runnable;
specifically add a trailing "\" after the KUBECONFIG=/kubeconfig line in the
podman run that sets -v $KUBECONFIG and -e KUBECONFIG, and likewise fix the
later podman run snippet that wraps the --skip argument (look for the podman run
lines and the --skip flag) so every fragmented line ends with "\" to maintain
proper shell continuation.
---
Nitpick comments:
In @.github/workflows/kustomize-validation.yml:
- Around line 19-43: The workflow currently pipes the installer from the master
branch (the curl line fetching "install_kustomize.sh") which installs the latest
kustomize and causes non-deterministic CI; change the curl invocation to pin a
specific kustomize version by either passing the version to the script (invoke
the script with "bash -s -- <VERSION>" instead of plain "bash") or by
downloading a release-tagged install script URL (replace the master URL with the
specific release tag) so that the installed kustomize version is deterministic;
update any references around MAX_ATTEMPTS/ATTEMPT and the sudo mv/kustomize
steps to remain compatible with the pinned installer behavior.
In @.github/workflows/release-testsuite.yaml:
- Line 26: The hardcoded tag value "4.22.999-snapshot" should be replaced with a
reusable env var; define RELEASE_TAG in the workflow's top-level env block (or
as a workflow_dispatch/input) and change the tags entry to reference it (use
GitHub Actions expression syntax like ${{ env.RELEASE_TAG }}), so update the
workflow file to declare RELEASE_TAG and replace the literal "4.22.999-snapshot"
with that env reference.
In `@hack/test-kustomize.sh`:
- Around line 38-45: The is_excluded function currently uses exact equality and
fails on ./ prefixes or nested paths; update is_excluded to normalize the
incoming dir and each EXCLUDED_DIRS entry by removing any leading ./ and
trailing slashes, then test exclusion by checking either exact equality or
prefix-match (excluded + "/" as a prefix) so directories under an excluded root
are also skipped; reference the is_excluded function and the EXCLUDED_DIRS array
in your change and ensure the same normalization/prefix logic is applied
wherever is_excluded is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bca4010d-efe6-42eb-ac46-e5af0d588300
📒 Files selected for processing (41)
.github/workflows/kustomize-validation.yml.github/workflows/release-testsuite.yaml.konflux/bundle/bundle.konflux.Dockerfile.konflux/bundle/metadata/annotations.yaml.konflux/bundle/overlay/pin_images.in.yaml.konflux/bundle/overlay/release.in.yaml.konflux/catalog/bundle.builds.in.yaml.konflux/catalog/catalog-idms.yaml.konflux/catalog/catalog-template.in.yaml.konflux/container-build.args.konflux/must-gather/must-gather.konflux.Dockerfile.konflux/must-gather/rpms.lock.yaml.konflux/operator/konflux.Dockerfile.tekton/build-pipeline-must-gather.yaml.tekton/build-pipeline.yaml.tekton/fbc-pipeline.yaml.tekton/images-mirror-set.yaml.tekton/numaresources-must-gather-4-22-pull-request.yaml.tekton/numaresources-must-gather-4-22-push.yaml.tekton/numaresources-operator-4-22-pull-request.yaml.tekton/numaresources-operator-4-22-push.yaml.tekton/numaresources-operator-bundle-4-22-pull-request.yaml.tekton/numaresources-operator-bundle-4-22-push.yaml.tekton/numaresources-operator-fbc-4-22-pull-request.yaml.tekton/numaresources-operator-fbc-4-22-push.yamlMakefileREADME.tests.mdbundle/manifests/numaresources-operator.clusterserviceversion.yamlconfig/manager/kustomization.yamlconfig/manifests/bases/numaresources-operator.clusterserviceversion.yamlconfig/samples/nodetopology_v1_numaresourcesscheduler.yamlconfig/samples/nodetopology_v1alpha1_numaresourcesscheduler.yamldoc/examples/sched.debug.yamldoc/examples/sched.ha.yamldoc/examples/sched.nocache.yamldoc/examples/sched.yamlhack/test-kustomize.shinternal/api/features/types.gointernal/controller/numaresourcesscheduler_controller_test.gopkg/numaresourcesscheduler/manifests/yaml/configmap.nrt.yamltest/e2e/must-gather/must_gather_suite_test.go
|
@sebrandon1: The following tests 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. |
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
Hey @sebrandon1,
if still relevant please rebase
4 participants
Similar to: openshift-kni/telco-reference#433
Changes:
kustomize buildappropriate files. Only runs againstmain.make test-kustomizewhich runs the script. Users can run this locally as well as there are safeguards to ensurekustomizeexists first.EXCLUDED_DIRS.Example run: