Migrating oc cli tests to ote #2181
Conversation
WalkthroughRemoves a trivial e2e test and adds a full end-to-end test suite, a comprehensive CLI/test utility library, embedded fixture extraction, and many YAML test fixtures; go.mod updated for dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
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: 7
🤖 Fix all issues with AI agents
In `@test/e2e/util.go`:
- Around line 1288-1305: applyResourceFromTemplate builds the wrong command by
calling oc.Run("oc") which yields a double "oc" (e.g., "oc oc process ...");
change the invocation to use the verb only (e.g., oc.Run("process", ...)) or
otherwise call oc.execPath directly when building the process command so the
constructed command is "oc process ..."; update the call sites inside
applyResourceFromTemplate (references: function applyResourceFromTemplate,
method oc.Run, field oc.execPath, use of exec.Command) to ensure the processed
template step runs "oc process ..." and not "oc oc process ...", preserving the
rest of the logic (namespace and KUBECONFIG env handling) and returning
cmd.Run() as before.
- Around line 1086-1094: In sha256File, don't defer file.Close() before
verifying os.Open succeeded; check err returned by os.Open(fileName) first and
handle/return the error (or call o.Expect after confirming file is non-nil),
then defer file.Close(); ensure subsequent io.Copy and hex.EncodeToString still
run when file is valid and propagate any errors from io.Copy instead of causing
a nil-pointer panic.
- Around line 1307-1332: The function applyResourceFromTemplate48681 incorrectly
invokes oc.Run("oc") which constructs an invalid command; replace that call with
exec.Command using oc.execPath and the constructed fullArgs (i.e., build cmd :=
exec.Command(oc.execPath, fullArgs...) and call cmd.Output()), and make sure to
set cmd.Env to include "KUBECONFIG="+oc.kubeconfig when oc.kubeconfig is
non-empty (and include any necessary namespace flags in fullArgs if required) so
the template processing output is captured correctly and returned or error
propagated.
- Around line 886-899: In getLatestPayload, the defer res.Body.Close() is
currently placed after the http.Get error check but before ioutil.ReadAll, which
can still leak the body if ReadAll errors; move the defer immediately after
confirming http.Get returned no error and res != nil (i.e., right after the err
check) so the response body is guaranteed to be closed on every return, and
ensure any early e2e.Failf paths occur after that placement; keep existing error
handling for ioutil.ReadAll and gjson parsing.
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml`:
- Around line 40-42: The memory request for the first container currently uses
the SI unit "40M" which is inconsistent with the second container's "40Mi" and
Kubernetes conventions; update the memory request value under the "requests:"
block (the entry currently "memory: 40M") to use the mebibyte unit "40Mi" so
both containers use the same unit and avoid quota calculation discrepancies.
In `@testdata/oc_cli/pod_mirror.yaml`:
- Around line 16-19: The shell loop condition in the args block currently uses
"[ ls /etc/foo/.dockerconfigjson ]" which always evaluates true; replace that
test with a proper file-existence check using the -f unary operator and negation
so the loop waits until /etc/foo/.dockerconfigjson exists (i.e., change the
while condition to test for "not -f /etc/foo/.dockerconfigjson" and keep the
surrounding "do sleep 1; done" and the subsequent exec oc adm release mirror
command intact).
In `@testdata/oc_cli/rc-match-service.yaml`:
- Around line 30-31: The volume entry named "config" under the "volumes" list is
missing a volume source and will fail validation; update the "config" volume
definition (the volumes -> name: config entry) to include an appropriate source
such as secret (e.g., secret: {secretName: ...}), configMap (e.g., configMap:
{name: ...}), or emptyDir, matching how the PodSpec mounts it (and ensure any
referenced secret/configMap exists and the mount path in the container matches).
🧹 Nitpick comments (3)
testdata/oc_cli/config_74099.yaml (1)
35-151: Consider security hardening for DeploymentConfig test fixture.The static analysis flags missing
allowPrivilegeEscalation: false(CKV_K8S_20). While this is acceptable for testdata captured from existing clusters, consider adding explicit security constraints if this fixture is meant to represent best practices:securityContext: allowPrivilegeEscalation: false capabilities: drop: - ALLGiven this appears to be a captured cluster configuration for test purposes, this is a minor concern.
testdata/oc_cli/stable-storage.yaml (1)
33-46: Consider explicit security settings for test consistency.The static analysis flags missing
allowPrivilegeEscalation: falseandrunAsNonRoot: true. While this is test data, adding explicit security settings ensures the test fixtures align with security best practices and can validate that workloads with proper security contexts function correctly.🔒 Optional: Add explicit security context settings
securityContext: capabilities: {} privileged: false + allowPrivilegeEscalation: false + runAsNonRoot: truetest/e2e/util.go (1)
11-11: Deprecatedio/ioutilpackage usage.The
io/ioutilpackage is deprecated since Go 1.16. Replace withosandiopackage equivalents:
ioutil.ReadAll→io.ReadAllioutil.ReadFile→os.ReadFileioutil.WriteFile→os.WriteFileioutil.ReadDir→os.ReadDir
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (176)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/CONTRIBUTINGis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/expect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/color.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/ssh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/termios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/SYNTAX.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/gjson.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/match.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/pretty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/logo.pngis excluded by!**/*.png,!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/sjson.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/block.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/const.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_generic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_noasm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/xor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/curve25519/curve25519.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/buffer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/certs.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/channel.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client_auth.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/common.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/connection.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/handshake.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/internal/bcrypt_pbkdf/bcrypt_pbkdf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/kex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/keys.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mac.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/messages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mlkem.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/session.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/ssh_gss.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/streamlocal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/tcpip.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/transport.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/NOTICE.txtis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/code_string.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/codes.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/connectivity/connectivity.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/component.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_others.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_unix.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_windows.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/serviceconfig/serviceconfig.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/protoadapt/convert.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (36)
go.modtest/e2e/cli.gotest/e2e/e2e.gotest/e2e/util.gotestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/OWNERStestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/auth.jsontestdata/oc_cli/build_66672.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/egressnetworkpolicy.yamltestdata/oc_cli/icsp60499.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/update_egressnetworkpolicy.yaml
💤 Files with no reviewable changes (1)
- test/e2e/e2e.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
testdata/oc_cli/initContainer66989.yamlgo.modtestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/config_74099.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/initContainer.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/OWNERStestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/auth.jsontestdata/oc_cli/icsp60499.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/update_egressnetworkpolicy.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/egressnetworkpolicy.yamltest/e2e/util.gotestdata/oc_cli/build_66672.yaml
🪛 Checkov (3.2.334)
testdata/oc_cli/initContainer66989.yaml
[medium] 1-33: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-33: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/only-rc.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/config_74099.yaml
[medium] 35-151: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/deploymentconfig_with_quota.yaml
[medium] 1-62: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/initContainer.yaml
[medium] 1-32: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-32: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/dc_66672.yaml
[medium] 1-19: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/stable-storage.yaml
[medium] 17-60: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 17-60: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/rc-match-service.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
testdata/oc_cli/auth.json
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (30)
testdata/oc_cli/dc_66672.yaml (1)
1-19: Test fixture looks good for CLI e2e testing.The DeploymentConfig manifest is well-formed with properly matching selector and template labels, and uses a pinned image digest for reproducibility.
Regarding the static analysis hint (CKV_K8S_20): the missing
allowPrivilegeEscalation: falseis a valid security best practice for production workloads, but is acceptable to omit in test fixtures unless the test specifically validates security context behavior.testdata/oc_cli/update_egressnetworkpolicy.yaml (1)
1-12: LGTM! Valid test manifest for EgressNetworkPolicy update scenario.The manifest structure is correct for testing OpenShift egress policy updates. The use of the same
metadata.nameas the base egressnetworkpolicy.yaml file is appropriate for testing resource update operations.testdata/oc_cli/egressnetworkpolicy.yaml (1)
1-12: LGTM! Valid test manifest for baseline EgressNetworkPolicy.The manifest structure is correct for OpenShift egress policy testing. The Allow rule for a specific /32 CIDR followed by a deny-all rule provides appropriate test isolation.
testdata/oc_cli/OWNERS (1)
1-3: LGTM!Standard OWNERS file format with designated approvers for the testdata directory.
testdata/oc_cli/auth.json (1)
1-7: LGTM - Dummy test credentials are appropriate.The base64 value decodes to
admin:123456, which is clearly placeholder test data. The static analysis false positive can be safely ignored since this is an intentional fixture for testing authentication flows, not real credentials.testdata/oc_cli/clusterresource_for_user.yaml (1)
1-15: LGTM!Valid ClusterResourceQuota manifest with reasonable test values. The annotation-based selector pattern is correct for user-scoped quotas.
testdata/oc_cli/mirror-from-filesystem.txt (1)
1-1: LGTM!Valid image mirror mapping format for filesystem-to-registry test scenarios.
testdata/oc_cli/config-images-75997.txt (1)
1-2: LGTM!Valid image mapping entries for multi-architecture test scenarios using a local registry destination.
testdata/oc_cli/testmapping.txt (1)
1-1: LGTM!Valid image mapping format for test fixtures.
testdata/oc_cli/initContainer66989.yaml (1)
1-33: LGTM!Well-structured Pod manifest for initContainer testing. The pinned image digests ensure test reproducibility. The static analysis security hints (CKV_K8S_20, CKV_K8S_23) are appropriately ignored for test fixtures where minimal security configuration is intentional.
testdata/oc_cli/initContainer.yaml (1)
1-32: LGTM!Valid Pod manifest for initContainer testing with 10-second sleep. Static analysis security hints are appropriately ignored for test fixtures.
testdata/oc_cli/only-rc.yaml (1)
1-31: LGTM!Well-structured ReplicationController manifest with properly configured secret volume mount. Static analysis security hints are appropriately ignored for test fixtures.
testdata/oc_cli/debugpod_48681.yaml (1)
1-39: Well-configured security context for test pod.The template properly enforces security constraints:
runAsNonRoot,seccompProfile: RuntimeDefault,allowPrivilegeEscalation: false, and droppedALLcapabilities. This is appropriate for testing secure pod configurations.testdata/oc_cli/icspFile63855.yaml (1)
1-12: LGTM - valid ICSP test fixture.The ImageContentSourcePolicy structure is correct for testing mirror configuration scenarios. The hardcoded EC2 hostname is acceptable for test fixtures.
testdata/oc_cli/build_66672.yaml (1)
1-14: LGTM - valid Build test fixture.The Build configuration is well-structured with a pinned image digest for reproducibility. Appropriate for testing source strategy builds.
testdata/oc_cli/case72217/crd-crontab-72217.yaml (1)
1-30: LGTM!The CRD manifest is well-structured and follows Kubernetes conventions. The schema, naming, and scope are appropriately defined for test purposes.
testdata/oc_cli/case72217/crd-customtask-72217.yaml (1)
1-30: LGTM!The CRD manifest follows Kubernetes conventions and is consistent with the other test CRDs in this PR. The schema structure is appropriately defined for test purposes.
go.mod (1)
34-34:goexpectversion is appropriate—no updates or security issues.The
goexpectdependency is pinned tov0.0.0-20210430020637, which is the final commit from the google/goexpect repository (April 30, 2021). The repository became archived in February 2023 and has no formal releases or newer versions available. No CVE entries or security vulnerabilities exist for this version. No action required.testdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yaml (1)
1-913: LGTM - Test fixture data for ImageStream scenarios.This is a comprehensive ImageStream test fixture capturing a multi-arch nightly release with documented RPM inconsistencies. The structure is valid and serves its purpose for CLI testing.
test/e2e/util.go (2)
49-66: LGTM - CLI wrapper structure.The CLI struct and constructor provide a clean fluent API for test command execution.
229-239: LGTM - Project setup with cleanup.Good use of
g.DeferCleanupto ensure test project cleanup.testdata/oc_cli/icsp60499.yaml (1)
1-12: LGTM - ICSP test fixture.Valid ImageContentSourcePolicy test fixture. The duplicate mirror entries may be intentional for testing deduplication or redundant configuration handling.
testdata/oc_cli/case72217/cr-cat-72217.yaml (1)
1-6: LGTM - Custom Resource test fixture.Simple and valid CR instance for testing CRD scenarios with the corresponding Cattoy72217 CRD.
testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
244-251: Inconsistent indentation in volume definition.The volume definition uses mixed tabs and irregular spacing, which differs from the rest of the file's formatting. While JSON parsers will handle this, it reduces readability and maintainability.
🔧 Suggested fix for consistent formatting
- "volumes": [ - { - "name": "mytest-volume", - "secret": { - "secretName": "my-secret" - } - } - ] + "volumes": [ + { + "name": "mytest-volume", + "secret": { + "secretName": "my-secret" + } + } + ]Likely an incorrect or invalid review comment.
testdata/oc_cli/case72217/cr-custom-72217.yaml (1)
1-6: LGTM!Valid custom resource manifest that correctly references the
example.com/v1API group andCustomtask72217kind defined in the corresponding CRD.testdata/oc_cli/case72217/cr-cron-72217.yaml (1)
1-6: LGTM!Valid custom resource manifest for
CronTab72217with correct API group reference.testdata/oc_cli/idmsFile64921.yaml (1)
1-18: LGTM!Valid
ImageDigestMirrorSetmanifest with appropriate localhost mirrors for testing. The explicitmirrorSourcePolicysettings are good practice.testdata/oc_cli/case72217/crd-cattoy-72217.yaml (1)
1-30: LGTM!Well-structured CRD manifest with proper OpenAPI v3 schema, correct API version, and all required fields.
testdata/oc_cli/idmsFile63855.yaml (1)
1-12: LGTM!Valid
ImageDigestMirrorSetmanifest. The hardcoded EC2 hostname is acceptable for test fixtures where actual connectivity isn't required during test execution.testdata/oc_cli/icspFile64920.yaml (1)
1-2: Consider updating to the current ImageDigestMirrorSet API if this is not specifically testing backward compatibility.The
operator.openshift.io/v1alpha1API version for ImageContentSourcePolicy is deprecated as of OpenShift 4.13 and replaced by ImageDigestMirrorSet (usingconfig.openshift.io/v1). While still supported, new test data should use the current stable API unless this file is intended to specifically test ICSP compatibility or migration paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func getLatestPayload(url string) string { | ||
| res, err := http.Get(url) | ||
| if err != nil { | ||
| e2e.Failf("unable to get http with error: %v", err) | ||
| } | ||
| body, err := ioutil.ReadAll(res.Body) | ||
| defer res.Body.Close() | ||
| if err != nil { | ||
| e2e.Failf("unable to parse the http result with error: %v", err) | ||
| } | ||
|
|
||
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | ||
| return pullSpec | ||
| } |
There was a problem hiding this comment.
Defer placement after error check can cause resource leak.
The defer res.Body.Close() is placed after error handling for http.Get but the body could still leak if ioutil.ReadAll fails since the defer comes after the first error check but before the read operation succeeds.
🐛 Recommended fix
func getLatestPayload(url string) string {
res, err := http.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
+ defer res.Body.Close()
body, err := ioutil.ReadAll(res.Body)
- defer res.Body.Close()
if err != nil {
e2e.Failf("unable to parse the http result with error: %v", err)
}📝 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.
| func getLatestPayload(url string) string { | |
| res, err := http.Get(url) | |
| if err != nil { | |
| e2e.Failf("unable to get http with error: %v", err) | |
| } | |
| body, err := ioutil.ReadAll(res.Body) | |
| defer res.Body.Close() | |
| if err != nil { | |
| e2e.Failf("unable to parse the http result with error: %v", err) | |
| } | |
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | |
| return pullSpec | |
| } | |
| func getLatestPayload(url string) string { | |
| res, err := http.Get(url) | |
| if err != nil { | |
| e2e.Failf("unable to get http with error: %v", err) | |
| } | |
| defer res.Body.Close() | |
| body, err := ioutil.ReadAll(res.Body) | |
| if err != nil { | |
| e2e.Failf("unable to parse the http result with error: %v", err) | |
| } | |
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | |
| return pullSpec | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 886 - 899, In getLatestPayload, the defer
res.Body.Close() is currently placed after the http.Get error check but before
ioutil.ReadAll, which can still leak the body if ReadAll errors; move the defer
immediately after confirming http.Get returned no error and res != nil (i.e.,
right after the err check) so the response body is guaranteed to be closed on
every return, and ensure any early e2e.Failf paths occur after that placement;
keep existing error handling for ioutil.ReadAll and gjson parsing.
| requests: | ||
| cpu: 50m | ||
| memory: 40M |
There was a problem hiding this comment.
Inconsistent memory unit notation.
Line 42 uses 40M (SI megabytes) while line 54 uses 40Mi (mebibytes). This inconsistency could cause confusion in quota calculations since 40M ≈ 38.15Mi.
For consistency with Kubernetes conventions and the second container's specification:
requests:
cpu: 50m
- memory: 40M
+ memory: 40Mi📝 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.
| requests: | |
| cpu: 50m | |
| memory: 40M | |
| requests: | |
| cpu: 50m | |
| memory: 40Mi |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml` around lines 40 - 42, The
memory request for the first container currently uses the SI unit "40M" which is
inconsistent with the second container's "40Mi" and Kubernetes conventions;
update the memory request value under the "requests:" block (the entry currently
"memory: 40M") to use the mebibyte unit "40Mi" so both containers use the same
unit and avoid quota calculation discrepancies.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (176)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/CONTRIBUTINGis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/expect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/color.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/ssh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/termios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/SYNTAX.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/gjson.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/match.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/pretty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/logo.pngis excluded by!**/*.png,!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/sjson.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/block.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/const.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_generic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_noasm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/xor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/curve25519/curve25519.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/buffer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/certs.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/channel.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client_auth.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/common.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/connection.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/handshake.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/internal/bcrypt_pbkdf/bcrypt_pbkdf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/kex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/keys.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mac.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/messages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mlkem.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/session.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/ssh_gss.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/streamlocal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/tcpip.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/transport.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/NOTICE.txtis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/code_string.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/codes.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/connectivity/connectivity.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/component.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_others.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_unix.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_windows.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/serviceconfig/serviceconfig.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/protoadapt/convert.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
go.mod
🔇 Additional comments (3)
go.mod (3)
244-244: Investigate the necessity and compatibility of the ginkgo fork.The
replacedirective overridesgithub.com/onsi/ginkgo/v2with an OpenShift fork based on v2.6.1, while line 40 requires v2.27.2. This represents a significant version gap (21 minor versions behind upstream).Verify:
- Whether the OpenShift fork includes critical patches or OpenShift-specific features that justify the version gap
- If the fork is actively maintained and receives security updates
- Whether removing the replace directive and using upstream v2.27.2 directly is compatible with the codebase
- If this fork replacement is still necessary or can be removed
104-106: Verify compatibility and security for indirect dependency updates against release notes.Multiple indirect dependencies have been updated including gRPC, Protobuf, OpenTelemetry, and Kubernetes ecosystem packages. While web searches reveal no critical unpatched vulnerabilities in the versions mentioned (Protobuf v1.36.8, OpenTelemetry v1.36.0, containerd/errdefs v1.0.0), check the release notes and changelogs for:
- gRPC v1.72.2: Verify whether this version addresses known gRPC CVEs (CVE-2024-7246, CVE-2024-11407, and others)
- All listed updates: Confirm no breaking changes or compatibility issues with your target Kubernetes/gRPC versions
The shell script approach in the original comment has limitations (requires
ghCLI with proper authentication); a direct review of project release notes is more reliable.
40-41: No breaking changes or critical issues found in the test framework updates.Release notes for ginkgo/v2 (v2.21.0→v2.27.2) and gomega (v1.35.1→v1.38.2) show only maintenance fixes and backwards-compatible features. Notable additions include new environment variables (GINKGO_TIME_FORMAT, GINKGO_NO_COLOR) and optional reporting formats (JSON output options, SemVer filtering), but these do not affect existing tests. The updates are safe to apply.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
go.mod
Outdated
| github.com/go-ldap/ldap/v3 v3.4.11 | ||
| github.com/gonum/graph v0.0.0-20170401004347-50b27dea7ebb | ||
| github.com/google/go-cmp v0.7.0 | ||
| github.com/google/goexpect v0.0.0-20210430020637-ab937bf7fd6f |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google/goexpect github releases tagged versions
💡 Result:
google/goexpect has no GitHub releases — the repository is archived (read-only) and the Releases page shows "There aren’t any releases here." [1][2]
Sources:
- google/goexpect repository (archived). [1]
- google/goexpect Releases page (no releases). [2]
🏁 Script executed:
#!/bin/bash
# Check goexpect repository metadata and release information
# Using curl to query GitHub API without gh CLI
echo "=== Checking for goexpect releases ==="
curl -s https://api.github.com/repos/google/goexpect/releases | jq -r '.[].tag_name' | head -20
echo -e "\n=== Checking latest commits ==="
curl -s https://api.github.com/repos/google/goexpect/commits | jq -r '.[] | "\(.commit.committer.date) - \(.commit.message | split("\n")[0])"' | head -5
echo -e "\n=== Checking repository metadata ==="
curl -s https://api.github.com/repos/google/goexpect | jq '{archived: .archived, updated_at: .updated_at, forks: .forks_count, watchers: .watchers_count, open_issues: .open_issues_count}'Repository: openshift/oc
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check if there are any Go security advisories for goexpect
# Search for known vulnerability databases
echo "=== Searching for security information about goexpect ==="
# Check Go vulnerability database
curl -s https://pkg.go.dev/github.com/google/goexpect | grep -i "vulnerability\|security\|deprecated" || echo "No direct security info found"
# Check for any Go mod issues documentation
echo -e "\n=== Checking go.dev package page metadata ==="
curl -s https://pkg.go.dev/api/gomod/github.com/google/goexpect/v0.0.0-20210430020637-ab937bf7fd6f 2>/dev/null | jq '.' || echo "No metadata available for this specific pseudo-version"Repository: openshift/oc
Length of output: 168
Replace github.com/google/goexpect with an actively maintained alternative.
The google/goexpect repository is archived and has no tagged releases. The pseudo-version points to a commit from April 2021, and the library is no longer maintained. Using archived, unmaintained dependencies introduces maintainability and security risks.
Consider either switching to an actively maintained expect-style testing library or evaluating whether this dependency is still necessary.
There was a problem hiding this comment.
We have several dependencies and for the sustainability we can't add new ones. So it is better to add the tests that are using our current libraries without any new dependency.
There was a problem hiding this comment.
@ardaguclu Understood — I appreciate you clarifying the project's dependency policy. Keeping the dependency footprint minimal is a sensible approach for sustainability and maintainability.
If the test infrastructure can be refactored to use existing dependencies or built-in Go functionality instead of adding google/goexpect, tidwall/gjson, and tidwall/sjson, that would align with your guidelines. Let me know if you'd like any suggestions on alternative approaches using the current dependency set.
|
/test unit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@testdata/oc_cli/application-template-stibuild-with-mount-secret.json`:
- Around line 200-251: The container "ruby-helloworld" is missing a volumeMount
for the defined secret volume "mytest-volume", so add a "volumeMounts" entry
inside the container spec for the "ruby-helloworld" container that mounts
"mytest-volume" at an appropriate path (e.g., /etc/secret or /etc/my-secret)
with "readOnly": true; ensure the "volumeMounts" array is at the same level as
"env", "resources", and "imagePullPolicy" in the "containers" element so the
secret becomes accessible inside the container.
♻️ Duplicate comments (2)
testdata/oc_cli/rc-match-service.yaml (1)
30-31: Incomplete volume definition will fail validation.The
configvolume is missing a volume source (e.g.,secret,configMap,emptyDir). Kubernetes will reject this manifest.testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
40-42: Inconsistent memory unit notation.Line 42 uses
40Mwhile line 54 uses40Mi. This mismatch could cause subtle quota calculation differences (40M ≈ 38.15Mi). Align both containers to use40Mifor consistency.requests: cpu: 50m - memory: 40M + memory: 40Mi
🧹 Nitpick comments (4)
testdata/oc_cli/initContainer.yaml (1)
8-26: Consider adding security hardening to match other test fixtures.The static analysis correctly flags missing security constraints. Unlike
debugpod_48681.yamlin this PR which properly configures security, this manifest lacksallowPrivilegeEscalation: falseandrunAsNonRoot: true. The initContainer also has no securityContext.If this is intentional for testing permissive scenarios, consider adding a comment. Otherwise, aligning with the hardened pattern improves consistency.
🔒 Suggested security hardening
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" ... securityContext: - capabilities: {} - privileged: false + allowPrivilegeEscalation: false + capabilities: + drop: + - ALLtestdata/oc_cli/initContainer66989.yaml (1)
1-33: Same security hardening recommendation applies.This manifest is nearly identical to
initContainer.yamlwith a longer sleep duration. The same security context improvements suggested for that file apply here.The trailing
status: {}on line 33 is typically server-populated and unnecessary in a source manifest, though it's harmless.testdata/oc_cli/stable-storage.yaml (1)
31-32: Deprecated annotation present.The annotation
pod.alpha.kubernetes.io/initializedis from very old Kubernetes versions (pre-1.5) and is no longer functional. If this testdata is meant to test handling of legacy manifests, this is intentional. Otherwise, it can be removed.testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
244-251: Inconsistent indentation involumessection.This block uses tabs while the rest of the file uses spaces. Consider normalizing to spaces for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (94)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (36)
go.modtest/e2e/cli.gotest/e2e/e2e.gotest/e2e/util.gotestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/OWNERStestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/auth.jsontestdata/oc_cli/build_66672.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/egressnetworkpolicy.yamltestdata/oc_cli/icsp60499.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/update_egressnetworkpolicy.yaml
💤 Files with no reviewable changes (1)
- test/e2e/e2e.go
✅ Files skipped from review due to trivial changes (4)
- testdata/oc_cli/case72217/cr-custom-72217.yaml
- testdata/oc_cli/case72217/crd-cattoy-72217.yaml
- testdata/oc_cli/mirror-from-filesystem.txt
- testdata/oc_cli/idmsFile64921.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- testdata/oc_cli/OWNERS
- testdata/oc_cli/update_egressnetworkpolicy.yaml
- testdata/oc_cli/build_66672.yaml
- go.mod
- testdata/oc_cli/idmsFile63855.yaml
- testdata/oc_cli/pod_mirror.yaml
- testdata/oc_cli/testmapping.txt
- testdata/oc_cli/icspFile64920.yaml
- testdata/oc_cli/case72217/cr-cat-72217.yaml
- testdata/oc_cli/case72217/cr-cron-72217.yaml
- testdata/oc_cli/case72217/crd-crontab-72217.yaml
- testdata/oc_cli/config-images-75997.txt
- testdata/oc_cli/egressnetworkpolicy.yaml
- test/e2e/util.go
- testdata/oc_cli/icsp60499.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
testdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/only-rc.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/auth.jsontestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/rc-match-service.yaml
🪛 Checkov (3.2.334)
testdata/oc_cli/initContainer.yaml
[medium] 1-32: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-32: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/stable-storage.yaml
[medium] 17-60: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 17-60: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/only-rc.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/config_74099.yaml
[medium] 35-151: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/dc_66672.yaml
[medium] 1-19: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/deploymentconfig_with_quota.yaml
[medium] 1-62: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/initContainer66989.yaml
[medium] 1-33: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-33: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/rc-match-service.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
testdata/oc_cli/auth.json
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
testdata/oc_cli/clusterresource_for_user.yaml (1)
1-15: LGTM!This is a well-formed ClusterResourceQuota test fixture with valid structure, appropriate quota limits for testing, and correct annotation-based selector configuration.
testdata/oc_cli/auth.json (1)
1-7: LGTM — Test fixture with placeholder credentials.The static analysis warning is a false positive. The base64 value decodes to
admin:123456, which is clearly a dummy credential for test purposes, appropriately located in thetestdata/directory.testdata/oc_cli/case72217/crd-customtask-72217.yaml (1)
1-30: LGTM!This is a well-structured CRD test fixture. The manifest follows the Kubernetes apiextensions.k8s.io/v1 API specification correctly with proper versioning, schema definition, and naming configuration.
testdata/oc_cli/icspFile63855.yaml (1)
1-12: LGTM!The ImageContentSourcePolicy manifest is well-structured with valid apiVersion, kind, metadata, and spec fields. The two
repositoryDigestMirrorsentries correctly map upstream sources to the mirror registry.testdata/oc_cli/only-rc.yaml (1)
1-31: LGTM!The ReplicationController manifest is well-formed with a properly configured secret volume source. Suitable for test data purposes.
testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
45-56: LGTM!Second container configuration is well-defined with consistent resource units.
testdata/oc_cli/debugpod_48681.yaml (1)
1-39: Well-configured security context for test pod.The template properly enforces restricted Pod Security Standards with
runAsNonRoot,seccompProfile: RuntimeDefault,allowPrivilegeEscalation: false, and drops all capabilities. This is a good practice even for test fixtures.testdata/oc_cli/config_74099.yaml (1)
1-151: Testdata fixture looks appropriate for CLI testing.This manifest provides good coverage for testing DeploymentConfig with secrets, probes, and deployment strategy. The hardcoded values (clusterIP, route host, namespace) are acceptable for test fixtures.
Regarding the static analysis hint (CKV_K8S_20): the
allowPrivilegeEscalationwarning is not actionable here since this is testdata for CLI validation, not a production deployment manifest.testdata/oc_cli/dc_66672.yaml (1)
1-19: Minimal DeploymentConfig fixture is valid.This provides a clean minimal manifest for testing basic DeploymentConfig operations. The static analysis warning (CKV_K8S_20) is not applicable for test fixtures.
testdata/oc_cli/stable-storage.yaml (1)
1-60: StatefulSet fixture is suitable for testing.The manifest correctly defines a headless Service for StatefulSet DNS and includes PVC templates for storage testing. The static analysis warnings (CKV_K8S_20, CKV_K8S_23) are not actionable for test fixtures.
testdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yaml (1)
1-913: ImageStream fixture is appropriate for release image testing.This comprehensive release ImageStream snapshot provides excellent coverage for testing
occommands related to image operations (mirror, extract, info). The embedded inconsistency annotations accurately reflect real-world release scenarios and will help validate CLI handling of such metadata.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "spec": { | ||
| "containers": [ | ||
| { | ||
| "name": "ruby-helloworld", | ||
| "image": "origin-ruby-sample", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8080, | ||
| "protocol": "TCP" | ||
| } | ||
| ], | ||
| "env": [ | ||
| { | ||
| "name": "ADMIN_USERNAME", | ||
| "value": "${ADMIN_USERNAME}" | ||
| }, | ||
| { | ||
| "name": "ADMIN_PASSWORD", | ||
| "value": "${ADMIN_PASSWORD}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_USER", | ||
| "value": "${MYSQL_USER}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_PASSWORD", | ||
| "value": "${MYSQL_PASSWORD}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_DATABASE", | ||
| "value": "${MYSQL_DATABASE}" | ||
| } | ||
| ], | ||
| "resources": {}, | ||
| "terminationMessagePath": "/dev/termination-log", | ||
| "imagePullPolicy": "IfNotPresent", | ||
| "securityContext": { | ||
| "capabilities": {}, | ||
| "privileged": false | ||
| } | ||
| } | ||
| ], | ||
| "restartPolicy": "Always", | ||
| "dnsPolicy": "ClusterFirst", | ||
| "volumes": [ | ||
| { | ||
| "name": "mytest-volume", | ||
| "secret": { | ||
| "secretName": "my-secret" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
Missing volumeMounts for the secret volume.
The volumes section defines mytest-volume backed by secret my-secret, but the container ruby-helloworld lacks a corresponding volumeMounts entry. The secret won't be accessible inside the container.
Given the template name emphasizes "with-mount-secret", this appears unintentional.
Proposed fix
"resources": {},
"terminationMessagePath": "/dev/termination-log",
"imagePullPolicy": "IfNotPresent",
+ "volumeMounts": [
+ {
+ "name": "mytest-volume",
+ "mountPath": "/etc/secret-volume",
+ "readOnly": true
+ }
+ ],
"securityContext": {
"capabilities": {},
"privileged": false
}📝 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.
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "ruby-helloworld", | |
| "image": "origin-ruby-sample", | |
| "ports": [ | |
| { | |
| "containerPort": 8080, | |
| "protocol": "TCP" | |
| } | |
| ], | |
| "env": [ | |
| { | |
| "name": "ADMIN_USERNAME", | |
| "value": "${ADMIN_USERNAME}" | |
| }, | |
| { | |
| "name": "ADMIN_PASSWORD", | |
| "value": "${ADMIN_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_USER", | |
| "value": "${MYSQL_USER}" | |
| }, | |
| { | |
| "name": "MYSQL_PASSWORD", | |
| "value": "${MYSQL_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_DATABASE", | |
| "value": "${MYSQL_DATABASE}" | |
| } | |
| ], | |
| "resources": {}, | |
| "terminationMessagePath": "/dev/termination-log", | |
| "imagePullPolicy": "IfNotPresent", | |
| "securityContext": { | |
| "capabilities": {}, | |
| "privileged": false | |
| } | |
| } | |
| ], | |
| "restartPolicy": "Always", | |
| "dnsPolicy": "ClusterFirst", | |
| "volumes": [ | |
| { | |
| "name": "mytest-volume", | |
| "secret": { | |
| "secretName": "my-secret" | |
| } | |
| } | |
| ] | |
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "ruby-helloworld", | |
| "image": "origin-ruby-sample", | |
| "ports": [ | |
| { | |
| "containerPort": 8080, | |
| "protocol": "TCP" | |
| } | |
| ], | |
| "env": [ | |
| { | |
| "name": "ADMIN_USERNAME", | |
| "value": "${ADMIN_USERNAME}" | |
| }, | |
| { | |
| "name": "ADMIN_PASSWORD", | |
| "value": "${ADMIN_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_USER", | |
| "value": "${MYSQL_USER}" | |
| }, | |
| { | |
| "name": "MYSQL_PASSWORD", | |
| "value": "${MYSQL_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_DATABASE", | |
| "value": "${MYSQL_DATABASE}" | |
| } | |
| ], | |
| "resources": {}, | |
| "terminationMessagePath": "/dev/termination-log", | |
| "imagePullPolicy": "IfNotPresent", | |
| "volumeMounts": [ | |
| { | |
| "name": "mytest-volume", | |
| "mountPath": "/etc/secret-volume", | |
| "readOnly": true | |
| } | |
| ], | |
| "securityContext": { | |
| "capabilities": {}, | |
| "privileged": false | |
| } | |
| } | |
| ], | |
| "restartPolicy": "Always", | |
| "dnsPolicy": "ClusterFirst", | |
| "volumes": [ | |
| { | |
| "name": "mytest-volume", | |
| "secret": { | |
| "secretName": "my-secret" | |
| } | |
| } | |
| ] |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/application-template-stibuild-with-mount-secret.json` around
lines 200 - 251, The container "ruby-helloworld" is missing a volumeMount for
the defined secret volume "mytest-volume", so add a "volumeMounts" entry inside
the container spec for the "ruby-helloworld" container that mounts
"mytest-volume" at an appropriate path (e.g., /etc/secret or /etc/my-secret)
with "readOnly": true; ensure the "volumeMounts" array is at the same level as
"env", "resources", and "imagePullPolicy" in the "containers" element so the
secret becomes accessible inside the container.
gangwgr
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
gangwgr
force-pushed
the
cli-tests
branch
13 times, most recently
from
9135593 to
fc528a8
Compare
|
/test e2e-aws-ovn-serial-2of2 |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/assign @ardaguclu |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Dropped more comments. Also could you please remove the unused files under testdata directory.
test/e2e/cli.go
Outdated
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Longduration-ConnectedOnly-NonPreRelease-Author:yinzhou-High-43032-oc adm release mirror generating correct imageContentSources when using --to and --to-release-image", func() { | ||
| if checkProxy(oc) { |
There was a problem hiding this comment.
Can we remove this check at all?. We don't run behind proxy?
There was a problem hiding this comment.
Yes, but these cases runs in conformance suite also, if those run proxy based cluster then it will fail
test/e2e/cli.go
Outdated
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-High-44797-Could define a Command for DC", func() { | ||
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.11") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") && !isEnabledCapability(oc, "DeploymentConfig") { |
There was a problem hiding this comment.
We don't need this check anymore?, we are in 4.22?
test/e2e/cli.go
Outdated
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-High-43034-should not show signature verify error msgs while trying to mirror OCP image repository to [Flaky]", func() { | ||
| if checkProxy(oc) { |
There was a problem hiding this comment.
Should be removed with all the others?
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("Author:yinzhou-Medium-33648-must gather pod should not schedule on windows node", func() { |
There was a problem hiding this comment.
This test is not run on Windows already?, can be removed?
| } | ||
|
|
||
| g.By("Create test pod") | ||
| pod48681.createDebugPodUsingDefinition(oc) |
There was a problem hiding this comment.
What are we asserting here?. We don't check the returned value?. Can we remove this test?
There was a problem hiding this comment.
The assertions are hidden inside the helper function
// The helper function verifies:
// 1. Pod reaches Running state
// 2. oc debug -f command succeeds
// 3. Debug output contains "Starting pod/pod48681-debug"
test/e2e/cli.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(os.WriteFile(dockerTmpDirName, authContentBytes, 0640)).NotTo(o.HaveOccurred()) | ||
|
|
||
| //_, outErr, err := oc.WithoutNamespace().WithoutKubeconf().Run("adm").Args("release", "info", image).Outputs() |
There was a problem hiding this comment.
Can we remove these comments?
test/e2e/cli.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(os.WriteFile(dockerTmpDirName, authContentBytes, 0640)).NotTo(o.HaveOccurred()) | ||
|
|
||
| //_, outErr, err := oc.WithoutNamespace().WithoutKubeconf().Run("adm").Args("release", "info", image).Outputs() |
|
updated pr |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@test/e2e/util.go`:
- Around line 1213-1219: getClusterRegion currently indexes
getWorkersList(oc)[0] which will panic if the returned slice is empty; update
the getClusterRegion function to check the length of the slice returned by
getWorkersList(oc) before indexing, log a clear message (using e2e.Logf or
process logger) and return an empty string (or a sentinel/error value) when no
worker nodes are found to avoid panics; keep the rest of the logic (calling
oc.AsAdmin().WithoutNamespace().Run(...)) only when a worker node is available.
- Around line 959-965: The checkPodmanCred function builds the auth.json path
incorrectly by concatenating XDG_RUNTIME_DIR and "containers/auth.json" without
a path separator; update checkPodmanCred to construct the path using
filepath.Join (or explicitly insert the separator) when forming the auth.json
path, then use os.Stat on that joined path and return false only if
os.IsNotExist(err); reference function name: checkPodmanCred and the auth.json
path construction.
In `@test/testdata/oc_cli/case72217/cr-cron-72217.yaml`:
- Around line 1-6: The CR uses an unsupported field spec.test which will be
pruned because the CronTab72217 CRD (crd-crontab-72217.yaml) only defines
spec.cronSpec, spec.image, and spec.replicas; fix by either removing spec.test
from the CR (cr-cron-72217.yaml) to match the CRD or by adding spec.test to the
CRD schema in crd-crontab-72217.yaml (update the structural OpenAPI schema under
CronTab72217 to include a test property with the appropriate type and
description) so the field is accepted and not pruned.
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml`:
- Around line 1-6: The CR sets an undefined spec field `spec.test` on
Customtask72217 (object name my-new-customtask-object) which will be pruned;
either remove the `spec.test` entry from the CR or add `test` to the
Customtask72217 CRD (crd-customtask-72217.yaml) schema under the spec properties
(including type and any validation/defaults or preservation settings) so the
field is recognized and persisted by Kubernetes.
In `@test/testdata/oc_cli/config_74099.yaml`:
- Around line 65-137: The container spec for the container named
"rails-postgresql-example" is missing container-level securityContext entries
required for restricted PSA/SCC; update the container definition inside the pod
template (the container with name rails-postgresql-example) to include a
securityContext that sets allowPrivilegeEscalation: false and runAsNonRoot: true
(and optionally ensure a non-zero runAsUser is set elsewhere if required by
policy). This change should be applied to the container-level block (not the
pod-level securityContext) so the container enforces non-root and no privilege
escalation.
In `@test/testdata/oc_cli/initContainer66989.yaml`:
- Around line 8-22: The initContainer named "wait" and the app container
"hello-pod" lack securityContext settings to enforce non-root and disable
privilege escalation; update both container specs to add a securityContext that
sets allowPrivilegeEscalation: false and runAsNonRoot: true (optionally also set
a non-root runAsUser e.g., 1000) so both initContainers[0].securityContext and
containers[0].securityContext explicitly disable privilege escalation and
require running as non-root.
♻️ Duplicate comments (9)
test/testdata/oc_cli/only-rc.yaml (1)
14-27: Add restricted PSA/SCC controls to the RC container.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this RC can be rejected on restricted clusters.✅ Proposed fix
image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" name: origin + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true ports: - containerPort: 8443 name: openshifttest/testdata/oc_cli/initContainer.yaml (1)
8-22: Harden init and app containers for restricted PSA/SCC.
Both containers should explicitly disable privilege escalation and require non-root.✅ Proposed fix
initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" imagePullPolicy: IfNotPresent name: hello-pod ports: - containerPort: 8080 protocol: TCP resources: {} securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/stable-storage.yaml (1)
35-45: Add restricted PSA/SCC controls to the StatefulSet container.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this pod can be rejected on restricted clusters.✅ Proposed fix
securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/dc_66672.yaml (1)
16-19: AddallowPrivilegeEscalation: falseto avoid restricted PSA/SCC failures.This fixture may be rejected on locked-down clusters without explicitly disabling privilege escalation.
🔧 Proposed fix
containers: - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 name: default-container + securityContext: + allowPrivilegeEscalation: false resources: {}test/testdata/oc_cli/rc-match-service.yaml (1)
30-31: Fix invalidconfigvolume (missing volume source).A volume must specify a source (secret/configMap/emptyDir); otherwise this RC will fail validation.
🐛 Proposed fix (example using configMap)
volumes: - name: config + configMap: + name: master-configtest/testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
32-55: Disable privilege escalation on both containers.Without this, restricted PSA/SCC policies can reject the pods and break CLI tests.
🔧 Proposed fix
- image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 imagePullPolicy: Always name: hello-openshift + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60m @@ - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 imagePullPolicy: Always name: hello-openshift2 + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60mtest/testdata/fixtures.go (1)
116-127: SurfaceWalkDirerrors instead of ignoring them.Silent failures can return partial fixtures and make test failures harder to diagnose.
🔧 Proposed fix
func ListFixtures() []string { var fixtures []string - fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { + if err := fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } if !d.IsDir() { fixtures = append(fixtures, path) } return nil - }) + }); err != nil { + panic(fmt.Sprintf("failed to list fixtures: %v", err)) + } return fixtures }test/testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
201-251: Mount the secret volume in the frontend container.
mytest-volumeis defined but never mounted, so the secret is not available in the container.🔧 Proposed fix
"resources": {}, "terminationMessagePath": "/dev/termination-log", "imagePullPolicy": "IfNotPresent", + "volumeMounts": [ + { + "name": "mytest-volume", + "mountPath": "/etc/secret-volume", + "readOnly": true + } + ], "securityContext": { "capabilities": {}, "privileged": false }test/e2e/util.go (1)
1141-1158: Deployment readiness check is effectively bypassed.
AdminKubeClient()returns a dummy client, so the readiness condition succeeds immediately and doesn’t observe real deployment state. This can make tests race/flaky.🔧 One way to check real status via oc CLI
- deployment, err := oc.AdminKubeClient().AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) - if err != nil { - if apierrors.IsNotFound(err) { - e2e.Logf("Waiting for availability of deployment/%s\n", name) - return false, nil - } - return false, err - } - if deployment.Status.AvailableReplicas == *deployment.Spec.Replicas && deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { - e2e.Logf("Deployment %s available (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + output, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( + "deployment", name, "-n", namespace, + "-o=jsonpath={.status.availableReplicas} {.status.updatedReplicas} {.spec.replicas}", + ).Output() + if err != nil { + e2e.Logf("Waiting for availability of deployment/%s\n", name) + return false, nil + } + parts := strings.Fields(strings.TrimSpace(output)) + if len(parts) < 3 { + return false, nil + } + available, _ := strconv.Atoi(parts[0]) + updated, _ := strconv.Atoi(parts[1]) + desired, _ := strconv.Atoi(parts[2]) + if available == desired && updated == desired { + e2e.Logf("Deployment %s available (%d/%d)\n", name, available, desired) return true, nil } - e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, available, desired) return false, nil })
test/e2e/util.go
Outdated
| func checkPodmanCred() bool { | ||
| currentRuntime := os.Getenv("XDG_RUNTIME_DIR") | ||
| _, err := os.Stat(currentRuntime + "containers/auth.json") | ||
| if os.IsNotExist(err) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
Fix incorrect path when checking Podman credentials.
The missing path separator makes the auth.json lookup incorrect and can return false negatives.
🔧 Proposed fix
func checkPodmanCred() bool {
currentRuntime := os.Getenv("XDG_RUNTIME_DIR")
- _, err := os.Stat(currentRuntime + "containers/auth.json")
+ if currentRuntime == "" {
+ return false
+ }
+ _, err := os.Stat(filepath.Join(currentRuntime, "containers", "auth.json"))
if os.IsNotExist(err) {
return false
}
return true
}🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 959 - 965, The checkPodmanCred function builds
the auth.json path incorrectly by concatenating XDG_RUNTIME_DIR and
"containers/auth.json" without a path separator; update checkPodmanCred to
construct the path using filepath.Join (or explicitly insert the separator) when
forming the auth.json path, then use os.Stat on that joined path and return
false only if os.IsNotExist(err); reference function name: checkPodmanCred and
the auth.json path construction.
| func getClusterRegion(oc *CLI) string { | ||
| e2e.Logf("Getting cluster region") | ||
| node := getWorkersList(oc)[0] | ||
| region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("Cluster region: %s", region) | ||
| return region |
There was a problem hiding this comment.
Guard against empty worker node list to avoid panics.
getWorkersList(oc)[0] will panic when no worker nodes exist (e.g., SNO or unusual labeling).
🔧 Proposed fix
func getClusterRegion(oc *CLI) string {
e2e.Logf("Getting cluster region")
- node := getWorkersList(oc)[0]
+ workers := getWorkersList(oc)
+ if len(workers) == 0 {
+ masters, err := GetMasterNodes(oc)
+ if err != nil || len(masters) == 0 {
+ e2e.Warningf("No nodes found to determine cluster region")
+ return ""
+ }
+ workers = masters
+ }
+ node := workers[0]
region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output()
o.Expect(err).NotTo(o.HaveOccurred())
e2e.Logf("Cluster region: %s", region)
return region
}🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 1213 - 1219, getClusterRegion currently
indexes getWorkersList(oc)[0] which will panic if the returned slice is empty;
update the getClusterRegion function to check the length of the slice returned
by getWorkersList(oc) before indexing, log a clear message (using e2e.Logf or
process logger) and return an empty string (or a sentinel/error value) when no
worker nodes are found to avoid panics; keep the rest of the logic (calling
oc.AsAdmin().WithoutNamespace().Run(...)) only when a worker node is available.
| apiVersion: "stable.example.com/v1" | ||
| kind: CronTab72217 | ||
| metadata: | ||
| name: my-new-cron-object | ||
| spec: | ||
| test: test |
There was a problem hiding this comment.
CR spec doesn’t match the CronTab72217 CRD schema.
Line 6 uses spec.test, but test/testdata/oc_cli/case72217/crd-crontab-72217.yaml only defines spec.cronSpec, spec.image, and spec.replicas. With structural schemas, spec.test will be pruned, which can break any test expecting it. Please align the CR to the schema or add test to the CRD.
💡 Option: extend the CRD schema to allow spec.test
# test/testdata/oc_cli/case72217/crd-crontab-72217.yaml
properties:
cronSpec:
type: string
image:
type: string
replicas:
type: integer
+ test:
+ type: string🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-cron-72217.yaml` around lines 1 - 6, The CR
uses an unsupported field spec.test which will be pruned because the
CronTab72217 CRD (crd-crontab-72217.yaml) only defines spec.cronSpec,
spec.image, and spec.replicas; fix by either removing spec.test from the CR
(cr-cron-72217.yaml) to match the CRD or by adding spec.test to the CRD schema
in crd-crontab-72217.yaml (update the structural OpenAPI schema under
CronTab72217 to include a test property with the appropriate type and
description) so the field is accepted and not pruned.
| apiVersion: "example.com/v1" | ||
| kind: Customtask72217 | ||
| metadata: | ||
| name: my-new-customtask-object | ||
| spec: | ||
| test: test |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'crd-customtask-72217.yaml' test/Repository: openshift/oc
Length of output: 112
🏁 Script executed:
cat -n test/testdata/oc_cli/case72217/crd-customtask-72217.yamlRepository: openshift/oc
Length of output: 940
Remove spec.test from the CR or add it to the CRD schema.
The Customtask72217 CRD (crd-customtask-72217.yaml) only defines three spec fields: cronSpec, image, and replicas. The spec.test field set in line 6 is not in the CRD schema and lacks explicit preservation settings, so it will likely be pruned by Kubernetes, causing any test assertions on it to fail.
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml` around lines 1 - 6, The
CR sets an undefined spec field `spec.test` on Customtask72217 (object name
my-new-customtask-object) which will be pruned; either remove the `spec.test`
entry from the CR or add `test` to the Customtask72217 CRD
(crd-customtask-72217.yaml) schema under the spec properties (including type and
any validation/defaults or preservation settings) so the field is recognized and
persisted by Kubernetes.
| containers: | ||
| - env: | ||
| - name: DATABASE_SERVICE_NAME | ||
| value: postgresql | ||
| - name: POSTGRESQL_USER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: database-user | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: database-password | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_DATABASE | ||
| value: root | ||
| - name: SECRET_KEY_BASE | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: keybase | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_MAX_CONNECTIONS | ||
| value: "100" | ||
| - name: POSTGRESQL_SHARED_BUFFERS | ||
| value: 12MB | ||
| - name: APPLICATION_DOMAIN | ||
| - name: APPLICATION_USER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: application-user | ||
| name: rails-postgresql-example | ||
| - name: APPLICATION_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: application-password | ||
| name: rails-postgresql-example | ||
| - name: RAILS_ENV | ||
| value: production | ||
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | ||
| imagePullPolicy: IfNotPresent | ||
| livenessProbe: | ||
| failureThreshold: 3 | ||
| httpGet: | ||
| path: /articles | ||
| port: 8080 | ||
| scheme: HTTP | ||
| initialDelaySeconds: 10 | ||
| periodSeconds: 10 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 3 | ||
| name: rails-postgresql-example | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| readinessProbe: | ||
| failureThreshold: 3 | ||
| httpGet: | ||
| path: /articles | ||
| port: 8080 | ||
| scheme: HTTP | ||
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 3 | ||
| resources: | ||
| limits: | ||
| memory: 512Mi | ||
| terminationMessagePath: /dev/termination-log | ||
| terminationMessagePolicy: File | ||
| dnsPolicy: ClusterFirst | ||
| restartPolicy: Always | ||
| schedulerName: default-scheduler | ||
| securityContext: {} |
There was a problem hiding this comment.
Add container securityContext for restricted PSA/SCC.
This DeploymentConfig container lacks allowPrivilegeEscalation: false and runAsNonRoot: true.
✅ Proposed fix
name: rails-postgresql-example
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
ports:
- containerPort: 8080
protocol: TCP📝 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.
| containers: | |
| - env: | |
| - name: DATABASE_SERVICE_NAME | |
| value: postgresql | |
| - name: POSTGRESQL_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-user | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-password | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_DATABASE | |
| value: root | |
| - name: SECRET_KEY_BASE | |
| valueFrom: | |
| secretKeyRef: | |
| key: keybase | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_MAX_CONNECTIONS | |
| value: "100" | |
| - name: POSTGRESQL_SHARED_BUFFERS | |
| value: 12MB | |
| - name: APPLICATION_DOMAIN | |
| - name: APPLICATION_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-user | |
| name: rails-postgresql-example | |
| - name: APPLICATION_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-password | |
| name: rails-postgresql-example | |
| - name: RAILS_ENV | |
| value: production | |
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | |
| imagePullPolicy: IfNotPresent | |
| livenessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 10 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| name: rails-postgresql-example | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| readinessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| resources: | |
| limits: | |
| memory: 512Mi | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| dnsPolicy: ClusterFirst | |
| restartPolicy: Always | |
| schedulerName: default-scheduler | |
| securityContext: {} | |
| containers: | |
| - env: | |
| - name: DATABASE_SERVICE_NAME | |
| value: postgresql | |
| - name: POSTGRESQL_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-user | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-password | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_DATABASE | |
| value: root | |
| - name: SECRET_KEY_BASE | |
| valueFrom: | |
| secretKeyRef: | |
| key: keybase | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_MAX_CONNECTIONS | |
| value: "100" | |
| - name: POSTGRESQL_SHARED_BUFFERS | |
| value: 12MB | |
| - name: APPLICATION_DOMAIN | |
| - name: APPLICATION_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-user | |
| name: rails-postgresql-example | |
| - name: APPLICATION_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-password | |
| name: rails-postgresql-example | |
| - name: RAILS_ENV | |
| value: production | |
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | |
| imagePullPolicy: IfNotPresent | |
| livenessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 10 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| name: rails-postgresql-example | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| readinessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| resources: | |
| limits: | |
| memory: 512Mi | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| dnsPolicy: ClusterFirst | |
| restartPolicy: Always | |
| schedulerName: default-scheduler | |
| securityContext: {} |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/config_74099.yaml` around lines 65 - 137, The container
spec for the container named "rails-postgresql-example" is missing
container-level securityContext entries required for restricted PSA/SCC; update
the container definition inside the pod template (the container with name
rails-postgresql-example) to include a securityContext that sets
allowPrivilegeEscalation: false and runAsNonRoot: true (and optionally ensure a
non-zero runAsUser is set elsewhere if required by policy). This change should
be applied to the container-level block (not the pod-level securityContext) so
the container enforces non-root and no privilege escalation.
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 30"] | ||
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false |
There was a problem hiding this comment.
Harden init and app containers for restricted PSA/SCC.
Both containers should explicitly disable privilege escalation and require non-root.
✅ Proposed fix
initContainers:
- name: wait
image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
command: ["/bin/sh", "-c", "sleep 30"]
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
containers:
- image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83"
imagePullPolicy: IfNotPresent
name: hello-pod
ports:
- containerPort: 8080
protocol: TCP
resources: {}
securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
capabilities: {}
privileged: false📝 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.
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 30"] | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| capabilities: {} | |
| privileged: false | |
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 30"] | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: {} | |
| privileged: false |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/initContainer66989.yaml` around lines 8 - 22, The
initContainer named "wait" and the app container "hello-pod" lack
securityContext settings to enforce non-root and disable privilege escalation;
update both container specs to add a securityContext that sets
allowPrivilegeEscalation: false and runAsNonRoot: true (optionally also set a
non-root runAsUser e.g., 1000) so both initContainers[0].securityContext and
containers[0].securityContext explicitly disable privilege escalation and
require running as non-root.
There was a problem hiding this comment.
@ardaguclu Acknowledged. The security hardening recommendation applies to this Pod manifest as well—both the wait initContainer and the hello-pod app container should include allowPrivilegeEscalation: false and runAsNonRoot: true in their securityContext blocks to comply with restricted PSA/SCC policies.
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
I've passed through some tests.
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: knarra@redhat.com | ||
| g.It("MicroShiftBoth-ROSA-OSD_CCS-ARO-Author:knarra-High-63855-Medium-64944-Verify oc image extract and oc adm release info throws error when both --icsp-file and -idms-file flag is used", func() { |
There was a problem hiding this comment.
We can drop this, this is verified by unit tests.
| } | ||
| }) | ||
|
|
||
| g.It("Author:yinzhou-ROSA-OSD_CCS-ARO-High-76116-Make sure oc could run on rhel with fips on", func() { |
There was a problem hiding this comment.
This is an important one. Did you check that this is skipped or executed.
| }) | ||
| AssertWaitPollNoErr(err, "oc image mirror fails even after waiting for about 120 seconds") | ||
| }) | ||
| g.It("Author:yinzhou-ROSA-OSD_CCS-ARO-Medium-76150-Make sure oc debug node has set HOST env var", func() { |
| o.Expect(removeErr).NotTo(o.HaveOccurred()) | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-Medium-72217-Should get warning when there is an identical short name for two or more resources", func() { |
test/e2e/cli.go
Outdated
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-54411-Low-21060-kubectl exec should work behind authenticated proxy", func() { |
test/e2e/cli.go
Outdated
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-21115-Use kubelet explain to see detailed documentation of resources", func() { |
There was a problem hiding this comment.
In our CI, there is similar test already https://github.com/openshift/origin/blob/main/test/extended/cli/explain.go, we can remove this.
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-11202-Use oc explain to see detailed documentation of resources", func() { |
There was a problem hiding this comment.
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("Author:yinzhou-NonHyperShiftHOST-ROSA-OSD_CCS-ARO-High-68405-oc process works well for cross-namespace template", func() { |
There was a problem hiding this comment.
Important one, we should keep it.
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Please remove all the tests I commented as "we can remove this" including the files they depend on under testdata.
test/e2e/cli.go
Outdated
| o.Expect(outputWarning).To(o.ContainSubstring("could also match lower priority resource crontabs72217.stable.example.com")) | ||
| }) | ||
|
|
||
| g.It("Author:yinzhou-ConnectedOnly-ROSA-OSD_CCS-ARO-High-75997-Make sure images with different tag but same layers could be mirrored correctly", func() { |
There was a problem hiding this comment.
Maybe we can remove this one?
test/e2e/cli.go
Outdated
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-54411-Low-21060-kubectl exec should work behind authenticated proxy", func() { |
test/e2e/cli.go
Outdated
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-21115-Use kubelet explain to see detailed documentation of resources", func() { |
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-11202-Use oc explain to see detailed documentation of resources", func() { |
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-Low-11147-Show RC information and indicate bad secrets reference in oc status", func() { |
test/e2e/cli.go
Outdated
|
|
||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("NonHyperShiftHOST-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Critical-51009-High-51017-oc adm release new support manifest list", func() { |
test/e2e/cli.go
Outdated
| } | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-Medium-50399-oc apply could update EgressNetworkPolicy resource", func() { |
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("NonHyperShiftHOST-ConnectedOnly-ROSA-OSD_CCS-ARO-Author:yinzhou-Medium-44061-Check the default registry credential path for oc", func() { |
test/e2e/cli.go
Outdated
| } | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("Author:yinzhou-NonPreRelease-Longduration-High-45308-check oc adm prune deployments command with the orphans options works well [Serial][Timeout:30m]", func() { |
test/e2e/cli.go
Outdated
| ) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-High-43034-should not show signature verify error msgs while trying to mirror OCP image repository to [Flaky]", func() { |
|
@ardaguclu Removed cases as per comments |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 151-155: The test currently only logs when regexp.MatchString
returns true, so mismatches pass silently; update the checks that call
regexp.MatchString (the ones using oc.Run("describe").Args("quota",
"compute-resources-42982") and the similar clusterresourcequota call around
lines 176-179) to assert the boolean result instead of conditionally logging —
i.e., capture matched, err from regexp.MatchString and replace the if-only
logging with an assertion such as o.Expect(matched).To(o.BeTrue()) (keeping the
existing o.Expect(err).NotTo(o.HaveOccurred()) for the regex call) so failures
fail the test and still include the output in the assertion message if desired.
- Around line 410-414: The test picks a pod by unstable list index using
oc.WithoutNamespace().Run("get").Args(...,
"-o=jsonpath={.items[1].metadata.name}"), which makes debugPodName flaky; change
the lookup to target the specific pod name or selector (e.g., "hello-pod-debug")
instead. Update the assignment to debugPodName so it requests the pod by
explicit name or by label/selector via
oc.WithoutNamespace().Run("get").Args("pods", "<pod-name-or-selector>", "-n",
oc.Namespace(), ...), then use that stable debugPodName in the subsequent
oc.WithoutNamespace().Run("describe") call.
- Around line 648-656: The test treats the command stdout stored in variable
output as a filename (exec.Command("bash", "-c", "cat "+output+" | awk ..."))
which is incorrect; change the
oc.AsAdmin().WithoutNamespace().Run("get").Args("events", "-n", projectN)
invocation to include the --no-headers flag and remove exec.Command usage, then
parse output in Go: split output into lines with strings.Split, for each
non-empty line use strings.Fields to get the first field and check for the
"unknown" timestamp; update the checks around variables output and result
accordingly to use the parsed fields instead of calling cat/awk.
In `@test/e2e/util.go`:
- Around line 716-720: The check for an empty nodeNameList is incorrect because
strings.Fields() returns an empty slice, not nil; replace the nil check with a
length check (use len(nodeNameList) == 0) where nodeNameList is evaluated (the
branch that currently reads "if nodeNameList == nil") so the function correctly
detects an empty result from strings.Fields() and returns false, nil.
- Around line 103-104: Handle the error returned by os.UserHomeDir() instead of
ignoring it: capture the error from os.UserHomeDir(), and if it is non-nil
either return/propagate the error from the calling function or build the
fallback using a reliable source such as os.Getenv("HOME") (e.g., if err != nil
{ home = os.Getenv("HOME") } or return an error), then use filepath.Join(home,
".kube", "config") as before; update the function containing the
os.UserHomeDir() call so it no longer silently produces "/.kube/config" when
home lookup fails.
♻️ Duplicate comments (14)
test/testdata/oc_cli/case72217/cr-custom-72217.yaml (1)
1-6: CR fieldspec.testisn’t declared in the Customtask72217 CRD.It will likely be pruned by the API server under structural schemas. Either remove it from the CR or add it to the CRD’s schema.
🧩 Proposed fix (remove from CR)
spec: - test: testtest/testdata/oc_cli/case72217/cr-cron-72217.yaml (1)
1-6:spec.testisn’t part of the CronTab72217 CRD schema.This field will be pruned; align the CR with the CRD or extend the CRD schema.
🧩 Proposed fix (remove from CR)
spec: - test: testtest/testdata/oc_cli/initContainer.yaml (1)
8-22: Add non-escalation and non-root constraints for restricted policies.Both the init container and app container are missing
allowPrivilegeEscalation: falseandrunAsNonRoot: true, which can fail under PSA/SCC “restricted”.✅ Proposed fix
initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" imagePullPolicy: IfNotPresent name: hello-pod @@ securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/deploymentconfig_with_quota.yaml (2)
32-55: SetallowPrivilegeEscalation: falseon both containers.Without this, pods can be rejected under restricted PSA/SCC.
🔧 Proposed fix
- image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 imagePullPolicy: Always name: hello-openshift + securityContext: + allowPrivilegeEscalation: false resources: @@ - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 imagePullPolicy: Always name: hello-openshift2 + securityContext: + allowPrivilegeEscalation: false resources:
40-42: Use consistent memory units (Mi) in requests.
40Mdiffers from40Miand can affect quota calculations.🧩 Proposed fix
requests: cpu: 50m - memory: 40M + memory: 40Mitest/testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
200-251: Secret volume is defined but never mounted.
mytest-volumeis declared but not mounted intoruby-helloworld, so the secret isn’t accessible.✅ Proposed fix
"resources": {}, "terminationMessagePath": "/dev/termination-log", "imagePullPolicy": "IfNotPresent", + "volumeMounts": [ + { + "name": "mytest-volume", + "mountPath": "/etc/secret-volume", + "readOnly": true + } + ], "securityContext": { "capabilities": {}, "privileged": false }test/testdata/fixtures.go (1)
116-127: Handlefs.WalkDirerrors inListFixtures.
Ignoring the WalkDir error can hide fixture traversal failures and return incomplete results.🔧 Suggested fix
func ListFixtures() []string { var fixtures []string - fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { + if err := fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } if !d.IsDir() { fixtures = append(fixtures, path) } return nil - }) + }); err != nil { + panic(fmt.Sprintf("failed to list fixtures: %v", err)) + } return fixtures }test/testdata/oc_cli/initContainer66989.yaml (1)
8-22: Add restricted PSA/SCC securityContext to init and app containers.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this pod can be rejected on restricted clusters.✅ Suggested hardening
initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 30"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" imagePullPolicy: IfNotPresent name: hello-pod ports: - containerPort: 8080 protocol: TCP resources: {} securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/rc-match-service.yaml (1)
30-31:configvolume is missing a source.
Kubernetes requires a source (secret,configMap,emptyDir, etc.). This manifest will fail to apply.🐛 Example fix (secret)
volumes: - name: config + secret: + secretName: mysecrettest/testdata/oc_cli/only-rc.yaml (1)
14-23: Add securityContext for restricted clusters.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this RC can be rejected under restricted PSA/SCC.✅ Suggested fix
containers: - args: - start - master - --config=/config/master-config.yaml image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" name: origin + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true ports: - containerPort: 8443 name: openshifttest/e2e/util.go (4)
959-965: Missing path separator causes incorrect auth.json lookup.The path construction
currentRuntime + "containers/auth.json"is missing the separator between the runtime directory and "containers", resulting in paths like/run/user/1000containers/auth.jsoninstead of/run/user/1000/containers/auth.json.🔧 Proposed fix
func checkPodmanCred() bool { currentRuntime := os.Getenv("XDG_RUNTIME_DIR") - _, err := os.Stat(currentRuntime + "containers/auth.json") + if currentRuntime == "" { + return false + } + _, err := os.Stat(filepath.Join(currentRuntime, "containers", "auth.json")) if os.IsNotExist(err) { return false } return true }
1044-1061: Movedefer res.Body.Close()before reading the body.The defer is placed after
ioutil.ReadAll, which means if the read fails, the defer won't have been registered yet. Move it immediately after the nil/error check forhttp.Get.🐛 Proposed fix
func getLatestPayload(url string) string { res, err := http.Get(url) if err != nil { e2e.Failf("unable to get http with error: %v", err) } - body, err := ioutil.ReadAll(res.Body) defer res.Body.Close() + body, err := io.ReadAll(res.Body) if err != nil { e2e.Failf("unable to parse the http result with error: %v", err) }
1141-1167:waitForDeploymentPodsToBeReadyalways returns success immediately due to dummy client.
oc.AdminKubeClient()returns adummyKubeClientCompletethat always returnsAvailableReplicas: 0,UpdatedReplicas: 0, andReplicas: 0(vianew(int32)). The condition0 == 0 && 0 == 0is always true, so this function returns success on first poll without checking actual deployment state.Either implement a real client or use the
ocCLI to query deployment status similar towaitForAvailableRsRunning.🔧 Proposed fix using oc CLI
func waitForDeploymentPodsToBeReady(oc *CLI, namespace string, name string) { e2e.Logf("Waiting for deployment %s to be ready in namespace %s", name, namespace) err := wait.Poll(20*time.Second, 300*time.Second, func() (done bool, err error) { - deployment, err := oc.AdminKubeClient().AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) + output, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( + "deployment", name, "-n", namespace, + "-o", "jsonpath={.spec.replicas} {.status.availableReplicas} {.status.updatedReplicas}", + ).Output() if err != nil { - if apierrors.IsNotFound(err) { - e2e.Logf("Waiting for availability of deployment/%s\n", name) - return false, nil - } - return false, err + e2e.Logf("Waiting for availability of deployment/%s\n", name) + return false, nil } - if deployment.Status.AvailableReplicas == *deployment.Spec.Replicas && deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { - e2e.Logf("Deployment %s available (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + parts := strings.Fields(output) + if len(parts) == 3 && parts[0] == parts[1] && parts[0] == parts[2] && parts[0] != "" && parts[0] != "0" { + e2e.Logf("Deployment %s available (%s/%s)\n", name, parts[1], parts[0]) return true, nil } - e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + e2e.Logf("Waiting for full availability of %s deployment\n", name) return false, nil })
1213-1219: Guard against empty worker node list to prevent index panic.
getWorkersList(oc)[0]will panic if no worker nodes exist (e.g., single-node OpenShift or unusual labeling).🔧 Proposed fix
func getClusterRegion(oc *CLI) string { e2e.Logf("Getting cluster region") - node := getWorkersList(oc)[0] + workers := getWorkersList(oc) + if len(workers) == 0 { + e2e.Warningf("No worker nodes found to determine cluster region") + return "" + } + node := workers[0] region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() o.Expect(err).NotTo(o.HaveOccurred()) e2e.Logf("Cluster region: %s", region) return region }
🧹 Nitpick comments (2)
test/e2e/util.go (2)
11-11: Replace deprecatedioutilpackage withioandos.The
ioutilpackage is deprecated since Go 1.16. Replace usages throughout the file:
ioutil.ReadAll→io.ReadAllioutil.ReadFile→os.ReadFileioutil.ReadDir→os.ReadDirioutil.WriteFile→os.WriteFile
623-631: Remove duplicategetRandomStringfunction.This is a duplicate of the exported
GetRandomStringfunction at lines 364-372. Use the exported version instead.♻️ Proposed fix - remove duplicate and update callers
-func getRandomString() string { - chars := "abcdefghijklmnopqrstuvwxyz0123456789" - seed := rand.New(rand.NewSource(time.Now().UnixNano())) - buffer := make([]byte, 8) - for index := range buffer { - buffer[index] = chars[seed.Intn(len(chars))] - } - return string(buffer) -}Then replace any internal calls to
getRandomString()withGetRandomString().
| g.By("Check if Init Containers present in debug pod output") | ||
| debugPodName, err := oc.WithoutNamespace().Run("get").Args("pods", "-n", oc.Namespace(), "-o=jsonpath={.items[1].metadata.name}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| Output, err := oc.WithoutNamespace().Run("describe").Args("pods", debugPodName, "-n", oc.Namespace()).Output() |
There was a problem hiding this comment.
Pod selection via .items[1] is order-dependent.
List ordering isn’t stable; this can pick the wrong pod and make the test flaky. Filter by name (e.g., hello-pod-debug) instead.
✅ Suggested fix
- debugPodName, err := oc.WithoutNamespace().Run("get").Args("pods", "-n", oc.Namespace(), "-o=jsonpath={.items[1].metadata.name}").Output()
- o.Expect(err).NotTo(o.HaveOccurred())
+ podsOut, err := oc.WithoutNamespace().Run("get").Args("pods", "-n", oc.Namespace(), "-o=name").Output()
+ o.Expect(err).NotTo(o.HaveOccurred())
+ debugPodName := ""
+ for _, pod := range strings.Fields(podsOut) {
+ if strings.Contains(pod, "hello-pod-debug") {
+ debugPodName = strings.TrimPrefix(pod, "pod/")
+ break
+ }
+ }
+ o.Expect(debugPodName).NotTo(o.BeEmpty(), "expected a debug pod")🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 410 - 414, The test picks a pod by unstable
list index using oc.WithoutNamespace().Run("get").Args(...,
"-o=jsonpath={.items[1].metadata.name}"), which makes debugPodName flaky; change
the lookup to target the specific pod name or selector (e.g., "hello-pod-debug")
instead. Update the assignment to debugPodName so it requests the pod by
explicit name or by label/selector via
oc.WithoutNamespace().Run("get").Args("pods", "<pod-name-or-selector>", "-n",
oc.Namespace(), ...), then use that stable debugPodName in the subsequent
oc.WithoutNamespace().Run("describe") call.
| home, _ := os.UserHomeDir() | ||
| return filepath.Join(home, ".kube", "config") |
There was a problem hiding this comment.
Silently ignored error from os.UserHomeDir() may produce invalid paths.
If os.UserHomeDir() fails, home will be empty and the returned path will be "/.kube/config", which is likely not the intended fallback.
🔧 Proposed fix
func KubeConfigPath() string {
if kc := os.Getenv("KUBECONFIG"); kc != "" {
return kc
}
- home, _ := os.UserHomeDir()
+ home, err := os.UserHomeDir()
+ if err != nil || home == "" {
+ return ""
+ }
return filepath.Join(home, ".kube", "config")
}📝 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.
| home, _ := os.UserHomeDir() | |
| return filepath.Join(home, ".kube", "config") | |
| home, err := os.UserHomeDir() | |
| if err != nil || home == "" { | |
| return "" | |
| } | |
| return filepath.Join(home, ".kube", "config") |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 103 - 104, Handle the error returned by
os.UserHomeDir() instead of ignoring it: capture the error from
os.UserHomeDir(), and if it is non-nil either return/propagate the error from
the calling function or build the fallback using a reliable source such as
os.Getenv("HOME") (e.g., if err != nil { home = os.Getenv("HOME") } or return an
error), then use filepath.Join(home, ".kube", "config") as before; update the
function containing the os.UserHomeDir() call so it no longer silently produces
"/.kube/config" when home lookup fails.
| if nodeNameList == nil { | ||
| e2e.Logf("Can't find must-gather pod now, and try next round") | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
Use len() instead of nil check for empty slice detection.
strings.Fields() returns an empty slice []string{} rather than nil when there are no fields. The nil check will never be true. Use len(nodeNameList) == 0 instead.
🔧 Proposed fix
nodeNameList = strings.Fields(output)
- if nodeNameList == nil {
+ if len(nodeNameList) == 0 {
e2e.Logf("Can't find must-gather pod now, and try next round")
return false, nil
}📝 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.
| if nodeNameList == nil { | |
| e2e.Logf("Can't find must-gather pod now, and try next round") | |
| return false, nil | |
| } | |
| return true, nil | |
| nodeNameList = strings.Fields(output) | |
| if len(nodeNameList) == 0 { | |
| e2e.Logf("Can't find must-gather pod now, and try next round") | |
| return false, nil | |
| } | |
| return true, nil |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 716 - 720, The check for an empty nodeNameList
is incorrect because strings.Fields() returns an empty slice, not nil; replace
the nil check with a length check (use len(nodeNameList) == 0) where
nodeNameList is evaluated (the branch that currently reads "if nodeNameList ==
nil") so the function correctly detects an empty result from strings.Fields()
and returns false, nil.
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Medium-44928-oc image mirror support registry which authorization server's url is different from registry url", func() { |
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("NonHyperShiftHOST-ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Medium-61607-oc image mirror always copy blobs if the target is file", func() { |
test/e2e/cli.go
Outdated
| }) | ||
| }) | ||
|
|
||
| var _ = g.Describe("[sig-cli] Workloads sos reports on Microshift", func() { |
There was a problem hiding this comment.
This needs to be renamed something else.
test/e2e/cli.go
Outdated
| ) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-LEVEL0-High-67387-oc new-app propagate containerPort information to the deployment if import-mode is default", func() { |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Just one comment. Other than that looks good
test/e2e/cli.go
Outdated
| checkPodStatus(oc, "openshift.io/deployer-pod-for.name=hello-openshift-1", "p42982-1", "Succeeded") | ||
| output, err = oc.AsAdmin().WithoutNamespace().Run("describe").Args("clusterresourcequota", "for-user42982").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if matched, _ := regexp.MatchString("requests.memory.*Ki.*8Gi", output); matched { |
There was a problem hiding this comment.
This test does not fail when matched is false
|
/test e2e-aws-oc-ote-serial |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 123-125: The skip condition in the g.It test uses && with
isEnabledCapability but due to operator precedence it only applies to the last
isBaselineCapsSet call; update the if condition so the ORed baseline checks are
grouped before applying the capability gate (e.g., wrap the series of
isBaselineCapsSet(...) || ... || isBaselineCapsSet(...) in parentheses) so the
combined baseline check is evaluated first and then && !isEnabledCapability(oc,
"DeploymentConfig") is applied; locate the condition in the test function (g.It)
where isBaselineCapsSet and isEnabledCapability are used and add the grouping to
fix the precedence.
- Around line 272-274: The deferred cmd.Process.Kill() can panic if
oc.Run(...).Background() fails and returns a nil cmd; update the Background()
call handling around the cmd, err return (the variables named cmd and err from
oc.Run("debug").Args(...).Background()) so you only defer killing the process
when cmd and cmd.Process are non-nil—either by checking err==nil before
installing the defer or by using a safe deferred closure that verifies cmd !=
nil && cmd.Process != nil before calling Kill(); ensure the
Expect(err).NotTo(HaveOccurred()) remains after the creation check so failures
still fail the test.
- Around line 466-487: The current if/else chain uses Ginkgo assertions
(o.Expect(...).To(o.BeTrue())) as branch conditions which panic on failure and
prevent subsequent branches from running; change the detection logic to use
plain boolean checks with strings.Contains(architectureStr, "...") for each
architecture (e.g., "x86_64", "aarch64", "ppc64le", "s390x") to select the
correct branch, and then perform assertions only on imageInfo inside each branch
(use o.Expect(strings.Contains(imageInfo, "<expected>")).To(o.BeTrue()) and
e2e.Logf/e2e.Failf as before), referencing the existing architectureStr and
imageInfo variables and preserving the same branch messages.
| g.It("ROSA-OSD_CCS-ARO-NonPreRelease-Longduration-Author:yinzhou-High-42982-Describe quota output should always show units [Timeout:30m]", func() { | ||
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") { | ||
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" |
There was a problem hiding this comment.
Fix baseline capability skip condition precedence.
&& !isEnabledCapability(...) only binds to the final isBaselineCapsSet check due to operator precedence, so the test can skip even when DeploymentConfig is enabled. Wrap the baseline-cap checks before applying the capability gate.
🔧 Proposed fix
- if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") {
+ if (isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11")) && !isEnabledCapability(oc, "DeploymentConfig") {📝 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.
| g.It("ROSA-OSD_CCS-ARO-NonPreRelease-Longduration-Author:yinzhou-High-42982-Describe quota output should always show units [Timeout:30m]", func() { | |
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") { | |
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" | |
| g.It("ROSA-OSD_CCS-ARO-NonPreRelease-Longduration-Author:yinzhou-High-42982-Describe quota output should always show units [Timeout:30m]", func() { | |
| if (isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11")) && !isEnabledCapability(oc, "DeploymentConfig") { | |
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 123 - 125, The skip condition in the g.It test
uses && with isEnabledCapability but due to operator precedence it only applies
to the last isBaselineCapsSet call; update the if condition so the ORed baseline
checks are grouped before applying the capability gate (e.g., wrap the series of
isBaselineCapsSet(...) || ... || isBaselineCapsSet(...) in parentheses) so the
combined baseline check is evaluated first and then && !isEnabledCapability(oc,
"DeploymentConfig") is applied; locate the condition in the test function (g.It)
where isBaselineCapsSet and isEnabledCapability are used and add the grouping to
fix the precedence.
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | ||
| defer cmd.Process.Kill() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
Guard deferred Process.Kill() when background start fails.
If Background() returns an error, cmd.Process can be nil and the deferred kill will panic.
🔧 Proposed fix
- cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background()
- defer cmd.Process.Kill()
- o.Expect(err).NotTo(o.HaveOccurred())
+ cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background()
+ o.Expect(err).NotTo(o.HaveOccurred())
+ if cmd != nil && cmd.Process != nil {
+ defer cmd.Process.Kill()
+ }📝 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.
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | |
| defer cmd.Process.Kill() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| if cmd != nil && cmd.Process != nil { | |
| defer cmd.Process.Kill() | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 272 - 274, The deferred cmd.Process.Kill() can
panic if oc.Run(...).Background() fails and returns a nil cmd; update the
Background() call handling around the cmd, err return (the variables named cmd
and err from oc.Run("debug").Args(...).Background()) so you only defer killing
the process when cmd and cmd.Process are non-nil—either by checking err==nil
before installing the defer or by using a safe deferred closure that verifies
cmd != nil && cmd.Process != nil before calling Kill(); ensure the
Expect(err).NotTo(HaveOccurred()) remains after the creation check so failures
still fail the test.
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| architectureStr := string(architecture) | ||
| if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch amd64") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch aarch64") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch ppc64le") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else { | ||
| if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the code exists at the specified location
cd /tmp && find . -name "cli.go" -path "*/e2e/*" 2>/dev/null | head -5Repository: openshift/oc
Length of output: 38
🏁 Script executed:
# List repository structure to locate the test file
git ls-files test/e2e/ | head -20Repository: openshift/oc
Length of output: 88
🏁 Script executed:
# Read the specific lines mentioned in the review
if git ls-files test/e2e/cli.go > /dev/null 2>&1; then
echo "File found. Reading lines 466-487:"
sed -n '466,487p' test/e2e/cli.go
fiRepository: openshift/oc
Length of output: 1109
🏁 Script executed:
# Get more context around these lines to understand the function
if git ls-files test/e2e/cli.go > /dev/null 2>&1; then
echo "Extended context (lines 450-500):"
sed -n '450,500p' test/e2e/cli.go | cat -n
fiRepository: openshift/oc
Length of output: 2727
🏁 Script executed:
# Search for similar patterns in the codebase
rg "if o\.Expect" test/e2e/ --context 2Repository: openshift/oc
Length of output: 1434
🏁 Script executed:
# Check imports to confirm the assertion library
head -50 test/e2e/cli.go | grep -E "import|gomega|ginkgo"Repository: openshift/oc
Length of output: 123
🌐 Web query:
Ginkgo Gomega o.Expect().To() return value conditional if statement
💡 Result:
Expect(x).To(matcher) does return a bool (true if it passed, false if it failed). [1]
However, under Ginkgo, a failed Gomega assertion calls Fail, and Fail panics to end the current spec, so you generally can’t use Expect(...).To(...) as a normal if condition for control flow (your code won’t reliably reach the else/next lines on failure). [2]
If you need a conditional check without failing the spec, use one of these patterns:
1) Call the matcher directly (pure boolean)
ok, err := Equal(42).Match(actual) // any GomegaMatcher works
if err != nil { /* matcher error */ }
if ok {
// passed
} else {
// failed (no Ginkgo failure triggered)
}2) Intercept assertion failures (keeps Gomega style)
if err := gomega.InterceptGomegaFailure(func() {
Expect(actual).To(Equal(42))
}); err == nil {
// passed
} else {
// failed; err contains the failure message
}InterceptGomegaFailure prevents registering the failure with the global fail handler and returns it as an error. [3]
Separate detection from assertion to fix architecture branching on non-x86 systems.
Using o.Expect(...).To(o.BeTrue()) in if conditions causes Ginkgo to panic on assertion failure, preventing else-if branches from being reached. On aarch64, ppc64le, and s390x systems, the first condition fails immediately and terminates the test, bypassing the correct architecture path.
Use plain strings.Contains() checks for detection logic, then assert only on the imageInfo result:
Suggested refactor
architectureStr := string(architecture)
- if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch amd64")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch aarch64")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch ppc64le")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else {
- if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch s390x")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- }
+ switch {
+ case strings.Contains(architectureStr, "x86_64"):
+ o.Expect(imageInfo).To(o.ContainSubstring("amd64"))
+ e2e.Logf("Found the expected Arch amd64")
+ case strings.Contains(architectureStr, "aarch64"):
+ o.Expect(imageInfo).To(o.ContainSubstring("arm64"))
+ e2e.Logf("Found the expected Arch aarch64")
+ case strings.Contains(architectureStr, "ppc64le"):
+ o.Expect(imageInfo).To(o.ContainSubstring("ppc64le"))
+ e2e.Logf("Found the expected Arch ppc64le")
+ case strings.Contains(architectureStr, "s390x"):
+ o.Expect(imageInfo).To(o.ContainSubstring("s390x"))
+ e2e.Logf("Found the expected Arch s390x")
+ default:
+ e2e.Failf("Unknown architecture: %s", architectureStr)
+ }📝 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.
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| architectureStr := string(architecture) | |
| if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch amd64") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch aarch64") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch ppc64le") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else { | |
| if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) { | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| architectureStr := string(architecture) | |
| switch { | |
| case strings.Contains(architectureStr, "x86_64"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("amd64")) | |
| e2e.Logf("Found the expected Arch amd64") | |
| case strings.Contains(architectureStr, "aarch64"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("arm64")) | |
| e2e.Logf("Found the expected Arch aarch64") | |
| case strings.Contains(architectureStr, "ppc64le"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("ppc64le")) | |
| e2e.Logf("Found the expected Arch ppc64le") | |
| case strings.Contains(architectureStr, "s390x"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("s390x")) | |
| e2e.Logf("Found the expected Arch s390x") | |
| default: | |
| e2e.Failf("Unknown architecture: %s", architectureStr) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 466 - 487, The current if/else chain uses
Ginkgo assertions (o.Expect(...).To(o.BeTrue())) as branch conditions which
panic on failure and prevent subsequent branches from running; change the
detection logic to use plain boolean checks with
strings.Contains(architectureStr, "...") for each architecture (e.g., "x86_64",
"aarch64", "ppc64le", "s390x") to select the correct branch, and then perform
assertions only on imageInfo inside each branch (use
o.Expect(strings.Contains(imageInfo, "<expected>")).To(o.BeTrue()) and
e2e.Logf/e2e.Failf as before), referencing the existing architectureStr and
imageInfo variables and preserving the same branch messages.
| return string(buffer) | ||
| } | ||
|
|
||
| func (registry *registry) createregistry(oc *CLI) serviceInfo { |
There was a problem hiding this comment.
This function is only used by oc image mirror with multi-arch images and --filter-by-os test. Maybe we can remove it as well as this registry struct?
test/e2e/util.go
Outdated
| return true | ||
| } | ||
|
|
||
| func checkPodmanCred() bool { |
test/e2e/util.go
Outdated
| return networkType | ||
| } | ||
|
|
||
| func checkDockerCred() bool { |
| @@ -0,0 +1,1636 @@ | |||
| package e2e | |||
There was a problem hiding this comment.
Could you please remove the unused functions in this util.go
| spec: | ||
| repositoryDigestMirrors: | ||
| - mirrors: | ||
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release |
There was a problem hiding this comment.
This is hardcoded value which might be unwanted. Can we remove this file or use better dns name?
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 10"] | ||
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false | ||
| terminationMessagePath: /dev/termination-log | ||
| volumeMounts: | ||
| - mountPath: /tmp | ||
| name: tmp | ||
| dnsPolicy: ClusterFirst |
There was a problem hiding this comment.
Can we remove dnsPolicy as well as suggested updates ^^
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 30"] | ||
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false |
test/testdata/oc_cli/only-rc.yaml
Outdated
| containers: | ||
| - args: | ||
| - start | ||
| - master | ||
| - --config=/config/master-config.yaml | ||
| image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" | ||
| name: origin | ||
| ports: | ||
| - containerPort: 8443 | ||
| name: openshift | ||
| volumeMounts: | ||
| - mountPath: /config | ||
| name: config | ||
| readOnly: true |
gangwgr
force-pushed
the
cli-tests
branch
2 times, most recently
from
d172801 to
3e0ec9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/testdata/oc_cli/case72217/crd-cattoy-72217.yaml`:
- Around line 29-30: The CRDs crd-cattoy-72217.yaml, crd-crontab-72217.yaml, and
crd-customtask-72217.yaml all declare the identical shortName `ct72217`, which
will conflict when applied; update the shortNames in these manifests to unique
values (for example change `ct72217` in crd-cattoy-72217.yaml to `cat72217`, in
crd-crontab-72217.yaml to `cron72217`, and in crd-customtask-72217.yaml to
`ctask72217`) or confirm the duplicate is intentional for collision tests and
add a comment explaining that intent.
🧹 Nitpick comments (1)
test/e2e/util.go (1)
11-11:ioutilpackage is deprecated.The
io/ioutilpackage is deprecated since Go 1.16. Consider usingioandospackages instead (e.g.,io.ReadAll,os.ReadFile,os.WriteFile).
| shortNames: | ||
| - ct72217 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, list all YAML files in the case72217 directory
ls -la test/testdata/oc_cli/case72217/ | grep -E "\.yaml$|\.yml$"Repository: openshift/oc
Length of output: 472
🏁 Script executed:
# Check all YAML files in the directory for shortName definitions
find test/testdata/oc_cli/case72217 -name "*.yaml" -o -name "*.yml" | head -20Repository: openshift/oc
Length of output: 370
🏁 Script executed:
# Search for ct72217 shortName across all files in the directory
grep -r "ct72217" test/testdata/oc_cli/case72217/Repository: openshift/oc
Length of output: 261
🏁 Script executed:
# Read the two specific CRD files to see the full context
echo "=== crd-cattoy-72217.yaml ===" && cat test/testdata/oc_cli/case72217/crd-cattoy-72217.yaml && echo -e "\n=== crd-crontab-72217.yaml ===" && cat test/testdata/oc_cli/case72217/crd-crontab-72217.yamlRepository: openshift/oc
Length of output: 1463
🏁 Script executed:
# Check the third CRD file to confirm completeness
cat test/testdata/oc_cli/case72217/crd-customtask-72217.yamlRepository: openshift/oc
Length of output: 730
Duplicate shortName ct72217 across three CRDs may cause conflicts.
Three CRDs in this directory define the same shortName: crd-cattoy-72217.yaml, crd-crontab-72217.yaml, and crd-customtask-72217.yaml. If all are applied to the same cluster, kubectl get ct72217 will be ambiguous. Verify if this is intentional for testing collision behavior, or update the shortNames to be unique (e.g., cat72217, cron72217, ctask72217).
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/crd-cattoy-72217.yaml` around lines 29 - 30,
The CRDs crd-cattoy-72217.yaml, crd-crontab-72217.yaml, and
crd-customtask-72217.yaml all declare the identical shortName `ct72217`, which
will conflict when applied; update the shortNames in these manifests to unique
values (for example change `ct72217` in crd-cattoy-72217.yaml to `cat72217`, in
crd-crontab-72217.yaml to `cron72217`, and in crd-customtask-72217.yaml to
`ctask72217`) or confirm the duplicate is intentional for collision tests and
add a comment explaining that intent.
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
I've detected many unused functions and files. Could you please remove them. Additionally, when I look at the sample run, I'm seeing that some tests are skipped https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_oc/2181/pull-ci-openshift-oc-main-e2e-aws-oc-ote/2016102965441466368. I don't think currently environment is disconnected, so either we need to fix the issues or remove those tests.
test/e2e/cli.go
Outdated
|
|
||
| // Extract secret and store it | ||
| extractTmpDirName := "/tmp/case64921" | ||
| err = os.MkdirAll(extractTmpDirName, 0755) |
There was a problem hiding this comment.
Needs to be less privileged rather than 0755
test/e2e/cli.go
Outdated
| } | ||
|
|
||
| extractTmpDirName := "/tmp/d71178" | ||
| err := os.MkdirAll(extractTmpDirName, 0755) |
test/e2e/cli.go
Outdated
| } | ||
| extractTmpDirName := "/tmp/case71273" | ||
| defer os.RemoveAll(extractTmpDirName) | ||
| err := os.MkdirAll(extractTmpDirName, 0755) |
test/testdata/fixtures.go
Outdated
|
|
||
| if d.IsDir() { | ||
| // Create directory | ||
| return os.MkdirAll(target, 0755) |
test/testdata/fixtures.go
Outdated
| } | ||
|
|
||
| // Create parent directory | ||
| if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil { |
test/e2e/util.go
Outdated
| } | ||
|
|
||
| // wait for DC to be ready | ||
| func waitForDeploymentconfigToBeReady(oc *CLI, namespace string, name string) { |
| @@ -0,0 +1,421 @@ | |||
| { | |||
test/testdata/oc_cli/only-rc.yaml
Outdated
| @@ -0,0 +1,31 @@ | |||
| apiVersion: v1 | |||
| @@ -0,0 +1,32 @@ | |||
| apiVersion: v1 | |||
| @@ -0,0 +1,23 @@ | |||
| server: | |||
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 856-857: The test writes sensitive credentials to a temp file via
OutputToFile("auth.dockerconfigjson") and never removes it; after the existing
error check (o.Expect(secretErr).NotTo(o.HaveOccurred())), delete the temp file
to avoid leaving secrets on disk by calling os.Remove on the path returned in
secretFile (or schedule a defer os.Remove(secretFile) immediately after the
successful OutputToFile call). Locate the OutputToFile call and the variable
secretFile to add the cleanup, ensuring the removal happens only after verifying
secretErr is nil.
In `@test/e2e/util.go`:
- Around line 267-276: The OutputToFile helper (CLICommand.OutputToFile)
currently writes sensitive pull-secret data using os.WriteFile with mode 0644,
making /tmp/auth.dockerconfigjson world-readable; change the file permission
mode passed to os.WriteFile from 0644 to 0600 so the file is created with owner
read/write only (restrictive permissions) while keeping the rest of the function
(filePath generation and return behavior) unchanged.
| secretFile, secretErr := oc.AsAdmin().WithoutNamespace().Run("get").Args("secret/pull-secret", "-n", "openshift-config", `--template={{index .data ".dockerconfigjson" | base64decode}}`).OutputToFile("auth.dockerconfigjson") | ||
| o.Expect(secretErr).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific lines and surrounding context
cat -n test/e2e/cli.go | sed -n '845,870p'Repository: openshift/oc
Length of output: 1586
🏁 Script executed:
# Let's search for OutputToFile to understand what it does
rg "OutputToFile" --type go -B 2 -A 2 | head -50Repository: openshift/oc
Length of output: 1685
🏁 Script executed:
# Check if there's any cleanup code related to auth.dockerconfigjson in the file
rg "auth.dockerconfigjson|defer.*Remove|os.Remove" test/e2e/cli.go -B 2 -A 2Repository: openshift/oc
Length of output: 2400
🏁 Script executed:
# Let's search for the OutputToFile method implementation
rg "func.*OutputToFile" --type go -B 2 -A 5Repository: openshift/oc
Length of output: 395
🏁 Script executed:
# Get the full OutputToFile implementation
sed -n '/^func (cmd \*CLICommand) OutputToFile/,/^}/p' test/e2e/util.goRepository: openshift/oc
Length of output: 314
Clean up the credentials file after use.
The OutputToFile() call creates a file at /tmp/auth.dockerconfigjson containing sensitive Docker credentials. This file is never removed and persists on disk after the test completes. Add cleanup immediately after the error check on line 857:
secretFile, secretErr := oc.AsAdmin().WithoutNamespace().Run("get").Args("secret/pull-secret", "-n", "openshift-config", `--template={{index .data ".dockerconfigjson" | base64decode}}`).OutputToFile("auth.dockerconfigjson")
o.Expect(secretErr).NotTo(o.HaveOccurred())
+ defer os.Remove(secretFile)🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 856 - 857, The test writes sensitive
credentials to a temp file via OutputToFile("auth.dockerconfigjson") and never
removes it; after the existing error check
(o.Expect(secretErr).NotTo(o.HaveOccurred())), delete the temp file to avoid
leaving secrets on disk by calling os.Remove on the path returned in secretFile
(or schedule a defer os.Remove(secretFile) immediately after the successful
OutputToFile call). Locate the OutputToFile call and the variable secretFile to
add the cleanup, ensuring the removal happens only after verifying secretErr is
nil.
| // OutputToFile runs the CLI command and writes output to a file | ||
| func (cmd *CLICommand) OutputToFile(filename string) (string, error) { | ||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| filePath := filepath.Join("/tmp", filename) | ||
| err = os.WriteFile(filePath, []byte(output), 0644) | ||
| return filePath, err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the function
cat -n test/e2e/util.go | sed -n '267,276p'Repository: openshift/oc
Length of output: 447
🏁 Script executed:
# Search for usages of OutputToFile to understand the context
rg "OutputToFile" --type go -B 2 -A 2Repository: openshift/oc
Length of output: 1685
🏁 Script executed:
# Check if this is indeed used for pull secrets
rg -i "pull.*secret|secret.*pull|credentials" test/e2e/ --type go -B 2 -A 2Repository: openshift/oc
Length of output: 4689
🏁 Script executed:
# Examine the broader context around the function definition
cat -n test/e2e/util.go | sed -n '250,290p'Repository: openshift/oc
Length of output: 1527
Use restrictive file permissions for sensitive credentials.
This helper writes pull-secret data to /tmp/auth.dockerconfigjson using 0644 permissions, making Docker registry credentials world-readable. Change to 0600 (owner read/write only).
🔧 Suggested fix
- err = os.WriteFile(filePath, []byte(output), 0644)
+ err = os.WriteFile(filePath, []byte(output), 0600)📝 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.
| // OutputToFile runs the CLI command and writes output to a file | |
| func (cmd *CLICommand) OutputToFile(filename string) (string, error) { | |
| output, err := cmd.Output() | |
| if err != nil { | |
| return "", err | |
| } | |
| filePath := filepath.Join("/tmp", filename) | |
| err = os.WriteFile(filePath, []byte(output), 0644) | |
| return filePath, err | |
| // OutputToFile runs the CLI command and writes output to a file | |
| func (cmd *CLICommand) OutputToFile(filename string) (string, error) { | |
| output, err := cmd.Output() | |
| if err != nil { | |
| return "", err | |
| } | |
| filePath := filepath.Join("/tmp", filename) | |
| err = os.WriteFile(filePath, []byte(output), 0600) | |
| return filePath, err |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 267 - 276, The OutputToFile helper
(CLICommand.OutputToFile) currently writes sensitive pull-secret data using
os.WriteFile with mode 0644, making /tmp/auth.dockerconfigjson world-readable;
change the file permission mode passed to os.WriteFile from 0644 to 0600 so the
file is created with owner read/write only (restrictive permissions) while
keeping the rest of the function (filePath generation and return behavior)
unchanged.
|
@gangwgr: 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. |
2 participants
This PR adds comprehensive CLI test infrastructure for the oc command, implementing a self-contained testing framework that avoids external test dependencies
New dependencies added:
Vendor updates: