CNTRLPLANE-1892: test(e2e-v2): add ginkgo-based v2 test suite #7192
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@csrwng: This pull request references CNTRLPLANE-1892 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a new e2ev2 end-to-end test suite and supporting infrastructure, updates Go module dependencies, and introduces test utilities, workload resolution logic, test assets, and build targets for running the new tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
approved
|
/test verify |
Add Ginkgo v2 testing framework and update related vendored dependencies including gomega matchers, protobuf, and supporting libraries needed for the new e2e v2 test suite. Signed-off-by: Cesar Wong <[email protected]> Assisted-by: Claude 3.7 Sonnet (via Claude Code)
csrwng
force-pushed
the
v2-tests-on-main
branch
from
076da23 to
cd2d957
Compare
|
/test verify |
|
/lgtm |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/e2e/v2/tests/assets/hostedcluster-base.yaml (1)
1-52: LGTM! Test asset structure is valid.The hardcoded domain names, release image, and secret references are acceptable for a test fixture. If this asset will be reused across different test environments, consider parameterizing these values.
test/e2e/util/crd.go (1)
21-30: Consider documenting the v1beta1 preference.The version selection logic prefers
v1beta1over other served versions but doesn't explain why. Adding a comment would improve maintainability.
📜 Review details
Configuration used: CodeRabbit 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 (283)
api/go.sumis excluded by!**/*.sumapi/vendor/golang.org/x/net/http2/http2.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!**/vendor/**api/vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**api/vendor/modules.txtis excluded by!**/vendor/**go.sumis excluded by!**/*.sumvendor/github.com/Masterminds/semver/v3/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/.golangci.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/SECURITY.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/collection.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/constraints.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/Masterminds/semver/v3/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/.editorconfigis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitattributesis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/LICENSE.txtis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/Taskfile.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/crypto.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/date.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/defaults.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/dict.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/functions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/list.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/numeric.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/reflect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/regex.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/strings.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-task/slim-sprig/v3/url.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/RELEASING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/config/deprecated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/core_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/decorator_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/deprecated_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_others.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/colorable_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/formatter/formatter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/automaxprocs.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroups.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cgroups2.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cpu_quota_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/cpu_quota_unsupported.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/mountpoint.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/runtime.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/automaxprocs/subsys.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/build/build_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/abort.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/command/program.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/boostrap_templates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/bootstrap_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generate_templates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/generators/generators_common.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/compile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/gocovmerge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/profiles_and_reports.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/run.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/test_suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/internal/verify_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/labels/labels_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/main.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/ginkgo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/import.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/outline/outline_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/unfocus/unfocus_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/delta_tracker.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/dependencies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hash.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/package_hashes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_cli_dependencies.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/ginkgo_t_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/around_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/counter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/failer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/focus.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/global/init.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/group.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/interrupt_handler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/interrupt_handler/sigquit_swallower_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/ordering.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_wasm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/output_interceptor_win.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/client_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/http_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/rpc_server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/parallel_support/server_handler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_bsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_wasm.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_report_win.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/progress_reporter_manager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/report_entry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson_event_writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/reporters/gojson_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/spec_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/testingtproxy/testing_t_proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/tree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/internal/writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/deprecated_reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/gojson_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/json_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/junit_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/reporter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporters/teamcity_report.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/reporting_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/table_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/around_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/code_location.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecated_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/deprecation_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/enum_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/file_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/flags.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/label_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/report_entry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/semver_filter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/ginkgo/v2/types/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.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/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/README.mdis excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/apic.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/decode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/emitterc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/encode.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/parserc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/readerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/resolve.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/scannerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/sorter.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/writerc.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yaml.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlh.gois excluded by!vendor/**,!**/vendor/**vendor/go.yaml.in/yaml/v3/yamlprivateh.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/PATENTSis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/cover/profile.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/inspector.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/iter.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/typeof.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/walk.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/gcexportdata/gcexportdata.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/gcexportdata/importer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/external.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/golist.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/golist_overlay.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/loadmode_string.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/packages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/packages/visit.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/callee.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/imports.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/map.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/methodsetcache.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/types/typeutil/ui.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/aliases/aliases.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/aliases/aliases_go122.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/core/event.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/core/export.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/core/fast.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/event.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/keys/keys.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/keys/standard.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/keys/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/event/label/label.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/bimport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/exportdata.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/gcimporter.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/iexport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/iimport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/iimport_go122.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/predeclared.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/support.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gcimporter/ureader_yes.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gocommand/invoke.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gocommand/invoke_notunix.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gocommand/invoke_unix.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gocommand/vendor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/gocommand/version.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/packagesinternal/packages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/codes.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/decoder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/encoder.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/flags.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/reloc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/support.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/sync.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/syncmarker_string.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/pkgbits/version.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/deps.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/import.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/manifest.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/stdlib/stdlib.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/common.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/coretype.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/free.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/normalize.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/termlist.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typeparams/typeterm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/classify_call.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/element.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/errorcode.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/errorcode_string.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/qualifier.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/recv.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/toonew.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/types.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/varkind.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/typesinternal/zerovalue.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/versions/features.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/versions/gover.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/versions/types.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/internal/versions/versions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (15)
api/go.mod(1 hunks)go.mod(5 hunks)test/e2e/util/crd.go(1 hunks)test/e2e/util/version.go(2 hunks)test/e2e/v2/internal/env_vars.go(1 hunks)test/e2e/v2/internal/test_context.go(1 hunks)test/e2e/v2/internal/workload_registry.go(1 hunks)test/e2e/v2/internal/workload_resolver.go(1 hunks)test/e2e/v2/tests/api_ux_validation_test.go(1 hunks)test/e2e/v2/tests/assets/hostedcluster-base.yaml(1 hunks)test/e2e/v2/tests/assets/karpenter-nodepool.yaml(1 hunks)test/e2e/v2/tests/assets/karpenter-workloads.yaml(1 hunks)test/e2e/v2/tests/assets/nodepool-base.yaml(1 hunks)test/e2e/v2/tests/control_plane_workloads_test.go(1 hunks)test/e2e/v2/tests/suite_test.go(1 hunks)
🧰 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:
test/e2e/util/crd.gotest/e2e/v2/tests/assets/karpenter-nodepool.yamltest/e2e/v2/internal/workload_registry.gotest/e2e/v2/tests/suite_test.gotest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/assets/hostedcluster-base.yamltest/e2e/v2/tests/assets/nodepool-base.yamltest/e2e/v2/tests/control_plane_workloads_test.gotest/e2e/util/version.gotest/e2e/v2/internal/workload_resolver.gotest/e2e/v2/tests/api_ux_validation_test.goapi/go.modtest/e2e/v2/tests/assets/karpenter-workloads.yamlgo.modtest/e2e/v2/internal/env_vars.go
🔇 Additional comments (11)
api/go.mod (1)
49-50: LGTM! Dependency updates align with test framework upgrade.The Ginkgo v2 and Gomega updates support the new test suite, and the transitive dependency bumps (protobuf, net) appear routine.
Also applies to: 62-62, 70-70
go.mod (1)
46-47: LGTM! Dependencies updated for Ginkgo v2 test framework.The direct dependencies on Ginkgo/Gomega v2 and the supporting indirect dependencies (semver, slim-sprig, tools) are appropriate for the new test suite.
Also applies to: 78-78, 122-122, 169-169, 245-245, 252-252, 257-257
test/e2e/util/crd.go (1)
40-93: LGTM! Schema traversal logic is sound.The recursive implementation correctly handles properties, array items, and schema combinators (AllOf/AnyOf/OneOf). The acknowledged limitation around $ref resolution is acceptable for common CRD validation use cases.
test/e2e/v2/tests/assets/karpenter-nodepool.yaml (1)
1-22: LGTM! Valid Karpenter NodePool configuration.The manifest correctly defines instance type requirements and capacity constraints for on-demand node provisioning.
test/e2e/v2/tests/assets/nodepool-base.yaml (1)
1-26: LGTM! Test asset with appropriate placeholder values.The manifest structure is valid. The placeholder values (some-cluster, a-profile, subnet-any) and
replicas: 0are appropriate for a base test fixture.test/e2e/v2/tests/assets/karpenter-workloads.yaml (1)
1-37: Deployment structure looks good otherwise.The pod anti-affinity, security context, and node selector configurations are correctly defined.
test/e2e/util/version.go (1)
87-91: LGTM! GinkgoAtLeast implementation is correct.The function properly skips tests when the release version doesn't meet the minimum requirement.
test/e2e/v2/internal/test_context.go (1)
90-139: LGTM!The test context initialization logic is clear and well-structured, with appropriate fallbacks for missing environment variables.
test/e2e/v2/internal/workload_resolver.go (3)
46-124: LGTM!The workload enumeration logic correctly filters CronJob-owned Jobs and provides deterministic ordering for stable test expectations.
213-289: Owner reference traversal logic is sound.The function correctly handles ReplicaSet → Deployment and Job → CronJob chains, with appropriate cycle detection and depth limits. The silent error handling during owner traversal (lines 249-251, 257-259) appears intentional for test robustness against transient API errors.
292-303: LGTM!Clean implementation using Kubernetes label selectors to fetch pods matching workload specifications.
| func SetReleaseVersionFromHostedCluster(ctx context.Context, hostedCluster *hyperv1.HostedCluster) error { | ||
| if hostedCluster.Status.Version == nil || len(hostedCluster.Status.Version.History) == 0 || hostedCluster.Status.Version.History[0].Version == "" { | ||
| fmt.Fprintf(ginkgo.GinkgoWriter, "WARNING: cannot determine release version from HostedCluster") | ||
| return nil | ||
| } | ||
| hcVersion := hostedCluster.Status.Version.History[0].Version | ||
| var err error | ||
| releaseVersion, err = semver.Parse(hcVersion) | ||
| if err != nil { | ||
| return fmt.Errorf("error parsing version: %v", err) | ||
| } | ||
| releaseVersion.Patch = 0 | ||
| releaseVersion.Pre = nil | ||
| releaseVersion.Build = nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Reconsider error handling for missing version data.
When the HostedCluster lacks version information, a warning is printed but the function returns nil. This leaves releaseVersion at its zero value (0.0.0), which will cause all subsequent GinkgoAtLeast checks to skip tests. This behavior may be intentional, but it could lead to confusing test failures or unexpected test skips without clear indication of the root cause.
Consider returning an error instead, or setting a documented default/minimum version to make the behavior more explicit.
🤖 Prompt for AI Agents
In test/e2e/util/version.go around lines 64 to 79, the function currently prints
a warning and returns nil when HostedCluster version info is missing, leaving
releaseVersion at its zero value and causing silent test skips; change the
behavior to return a descriptive error instead (e.g., "cannot determine release
version from HostedCluster: missing Status.Version or History") so callers can
fail or handle it explicitly; update any call sites/tests that expect nil to
handle the error or add a documented default version if a non-error flow is
desired.
| err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | ||
| Namespace: tc.ClusterNamespace, | ||
| Name: tc.ClusterName, | ||
| }, hostedCluster) |
There was a problem hiding this comment.
Use embedded context instead of context.Background().
The TestContext embeds context.Context, but line 56 uses context.Background(). This bypasses any cancellation or deadline semantics from the test context.
Apply this diff to use the embedded context:
hostedCluster := &hyperv1.HostedCluster{}
- err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{
+ err := tc.MgmtClient.Get(tc.Context, crclient.ObjectKey{
Namespace: tc.ClusterNamespace,
Name: tc.ClusterName,
}, hostedCluster)📝 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.
| err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | |
| Namespace: tc.ClusterNamespace, | |
| Name: tc.ClusterName, | |
| }, hostedCluster) | |
| err := tc.MgmtClient.Get(tc.Context, crclient.ObjectKey{ | |
| Namespace: tc.ClusterNamespace, | |
| Name: tc.ClusterName, | |
| }, hostedCluster) |
🤖 Prompt for AI Agents
In test/e2e/v2/internal/test_context.go around lines 56 to 59, replace the use
of context.Background() with the TestContext's embedded context (tc.Context) so
the Get call uses the test's cancellation/deadline semantics; update the call to
pass tc.Context instead of context.Background().
| currentUID := pod.UID | ||
| visited[currentUID] = true | ||
|
|
||
| owner := pod.OwnerReferences[0] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify assumption: pods have single controller owner.
The code assumes pod.OwnerReferences[0] is the controller owner. While this is typically true for managed pods, pods can theoretically have multiple owners.
Run the following script to check if any pods in control plane namespaces have multiple owner references:
🏁 Script executed:
#!/bin/bash
# Description: Check for pods with multiple owner references in HyperShift control plane namespaces
# Find all namespaces matching the control plane pattern
for ns in $(kubectl get namespaces -o json | jq -r '.items[].metadata.name' | grep -E '^(clusters|hypershift)-'); do
echo "Checking namespace: $ns"
kubectl get pods -n "$ns" -o json | jq -r '
.items[] |
select(.metadata.ownerReferences | length > 1) |
"Pod: \(.metadata.name) has \(.metadata.ownerReferences | length) owners: \([.metadata.ownerReferences[].kind] | join(", "))"
'
doneLength of output: 45
The kubectl approach won't work in the sandbox environment. Let me examine the codebase directly to understand the context better and search for documentation on Kubernetes pod owner references.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the workload_resolver.go file to understand the context around line 157
cat test/e2e/v2/internal/workload_resolver.go | head -220 | tail -70
Length of output: 1969
🌐 Web query:
Kubernetes pod owner references single controller owner pattern
💡 Result:
Short summary of the "single controller owner" pattern for Pod ownerReferences:
- ownerReferences is a list on a dependent (e.g., a Pod) that identifies its owners; the garbage collector uses that list to decide when to delete the dependent. [1]
- Exactly one ownerReference entry may be marked controller: true. That entry denotes the managing controller (the "controllerRef") — there cannot be more than one managing controller for a given object. This is used by controllers’ adoption logic to prevent multiple controllers from fighting over the same object. Other ownerReferences (if present) should have controller: false. [2]
- blockOwnerDeletion on an ownerReference (often set automatically by controllers) is used with foreground cascading deletion so that dependents can block deletion of the owner until they are removed; setting blockOwnerDeletion requires delete permission on the owner. Cascading-deletion behavior (Foreground/Background/Orphan) is controlled via DeleteOptions.propagationPolicy. Cross-namespace ownerReferences are disallowed. [1][3]
- Practical implications: when writing controllers (or using Deployments/ReplicaSets/StatefulSets), rely on controller-managed ownerReferences (controller=true) for adoption and lifecycle; let built-in controllers set blockOwnerDeletion automatically unless you explicitly need different deletion semantics. [2][3]
Sources:
- Kubernetes docs — Cascading deletion / garbage collection. [1]
- Discussion of controller=true (controllerRef) and adoption (StackOverflow summary). [2]
- Controller authoring / ownerReferences guidance (Kubebuilder / controller-runtime). [3]
Search for the owner with controller: true field instead of assuming index [0].
Kubernetes allows multiple owner references on a Pod, but exactly one is marked with controller: true, denoting the managing controller. The code assumes pod.OwnerReferences[0] is the controller owner, but this entry should be marked controller: true—it's not guaranteed to be at index [0]. The owner reference should be located by checking the controller field rather than relying on position.
🤖 Prompt for AI Agents
In test/e2e/v2/internal/workload_resolver.go around line 157, the code assumes
the controller owner is at pod.OwnerReferences[0]; instead, iterate the
pod.OwnerReferences to find the entry with Controller == true (or Controller !=
nil && *Controller == true) and use that as owner; handle cases where
OwnerReferences is empty or no controller-marked owner is found (return an
appropriate error or nil) and replace direct index access with the found owner
variable.
| resources: | ||
| requests: | ||
| cpu: "250m" | ||
| memory: 256M |
There was a problem hiding this comment.
Fix memory resource notation.
Line 33 uses 256M which is not valid Kubernetes quantity notation. Memory should use binary units like Mi, Gi, etc.
Apply this diff:
resources:
requests:
cpu: "250m"
- memory: 256M
+ memory: 256Mi📝 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.
| resources: | |
| requests: | |
| cpu: "250m" | |
| memory: 256M | |
| resources: | |
| requests: | |
| cpu: "250m" | |
| memory: 256Mi |
🤖 Prompt for AI Agents
In test/e2e/v2/tests/assets/karpenter-workloads.yaml around lines 30 to 33, the
memory request uses invalid Kubernetes notation "256M"; change it to a valid
binary unit such as 256Mi (e.g., set memory: 256Mi or "256Mi") so the manifest
uses correct Kubernetes quantity syntax.
| for _, workload := range workloads { | ||
| if workload.Type != "Deployment" { | ||
| continue | ||
| } | ||
|
|
||
| It(fmt.Sprintf("should not indicate rapid rollouts for %s", workload.Name), func() { | ||
| testCtx := getTestCtx() | ||
| hostedCluster := testCtx.GetHostedCluster() | ||
| if shouldSkipWorkloadForPlatform(workload, hostedCluster) { | ||
| Skip(fmt.Sprintf("workload %s is platform-specific and doesn't match cluster platform", workload.Name)) | ||
| } | ||
|
|
||
| deployment := &appsv1.Deployment{} | ||
| err := testCtx.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | ||
| Namespace: testCtx.ControlPlaneNamespace, | ||
| Name: workload.Name, | ||
| }, deployment) | ||
| if apierrors.IsNotFound(err) { | ||
| Skip(fmt.Sprintf("Deployment %s not found", workload.Name)) | ||
| } | ||
| Expect(err).NotTo(HaveOccurred(), "failed to get Deployment %s", workload.Name) | ||
|
|
||
| Expect(deployment.Generation).To(BeNumerically("<=", maxAllowedGeneration), | ||
| "Deployment %s has generation %d which exceeds max allowed %d", | ||
| workload.Name, deployment.Generation, maxAllowedGeneration) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix loop-variable capture so each spec tests the intended workload
All of the It closures inside these for _, workload := range workloads loops capture the same loop variable. Because Go reuses the loop variable, every spec ends up exercising only the final element of workloads, effectively disabling the coverage you’re aiming for. Please take a fresh copy of the workload inside the loop (e.g., workload := workload) before defining the closure—repeat this in every similar loop in the file—so each spec runs against the expected workload.
- for _, workload := range workloads {
- It(fmt.Sprintf("should not indicate rapid rollouts for %s", workload.Name), func() {
+ for _, workload := range workloads {
+ workload := workload
+ It(fmt.Sprintf("should not indicate rapid rollouts for %s", workload.Name), func() {
testCtx := getTestCtx()📝 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.
| for _, workload := range workloads { | |
| if workload.Type != "Deployment" { | |
| continue | |
| } | |
| It(fmt.Sprintf("should not indicate rapid rollouts for %s", workload.Name), func() { | |
| testCtx := getTestCtx() | |
| hostedCluster := testCtx.GetHostedCluster() | |
| if shouldSkipWorkloadForPlatform(workload, hostedCluster) { | |
| Skip(fmt.Sprintf("workload %s is platform-specific and doesn't match cluster platform", workload.Name)) | |
| } | |
| deployment := &appsv1.Deployment{} | |
| err := testCtx.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | |
| Namespace: testCtx.ControlPlaneNamespace, | |
| Name: workload.Name, | |
| }, deployment) | |
| if apierrors.IsNotFound(err) { | |
| Skip(fmt.Sprintf("Deployment %s not found", workload.Name)) | |
| } | |
| Expect(err).NotTo(HaveOccurred(), "failed to get Deployment %s", workload.Name) | |
| Expect(deployment.Generation).To(BeNumerically("<=", maxAllowedGeneration), | |
| "Deployment %s has generation %d which exceeds max allowed %d", | |
| workload.Name, deployment.Generation, maxAllowedGeneration) | |
| }) | |
| } | |
| }) | |
| for _, workload := range workloads { | |
| workload := workload | |
| if workload.Type != "Deployment" { | |
| continue | |
| } | |
| It(fmt.Sprintf("should not indicate rapid rollouts for %s", workload.Name), func() { | |
| testCtx := getTestCtx() | |
| hostedCluster := testCtx.GetHostedCluster() | |
| if shouldSkipWorkloadForPlatform(workload, hostedCluster) { | |
| Skip(fmt.Sprintf("workload %s is platform-specific and doesn't match cluster platform", workload.Name)) | |
| } | |
| deployment := &appsv1.Deployment{} | |
| err := testCtx.MgmtClient.Get(context.Background(), crclient.ObjectKey{ | |
| Namespace: testCtx.ControlPlaneNamespace, | |
| Name: workload.Name, | |
| }, deployment) | |
| if apierrors.IsNotFound(err) { | |
| Skip(fmt.Sprintf("Deployment %s not found", workload.Name)) | |
| } | |
| Expect(err).NotTo(HaveOccurred(), "failed to get Deployment %s", workload.Name) | |
| Expect(deployment.Generation).To(BeNumerically("<=", maxAllowedGeneration), | |
| "Deployment %s has generation %d which exceeds max allowed %d", | |
| workload.Name, deployment.Generation, maxAllowedGeneration) | |
| }) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In test/e2e/v2/tests/control_plane_workloads_test.go around lines 80 to 107, the
It closures capture the loop variable and end up all referencing the final
workload; fix by shadowing the loop variable inside the loop (e.g., create a new
local variable like workload := workload) before defining the It closure so each
closure captures its own copy; apply the same pattern to every similar for-range
loop in this file to ensure each spec tests the intended workload.
There was a problem hiding this comment.
Not needed for go 1.24
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Go version in go.mod files
echo "=== Checking Go version in root go.mod ==="
head -20 go.mod | grep -E "^go |^module "
echo -e "\n=== Checking Go version in api/go.mod ==="
head -20 api/go.mod | grep -E "^go |^module "
Length of output: 187
🌐 Web query:
Go 1.22 loop variable scoping changes
💡 Result:
Summary — what changed
- Starting in Go 1.22, loop variables are created per iteration instead of once per loop. That makes closures/goroutines inside a loop capture the iteration’s values (the intuitive behavior) instead of accidentally sharing one changing variable. [1][2][3]
When it applies
- The new semantics are opt‑in per module: only packages in modules that declare "go 1.22" (or later) in go.mod get the new behavior; older-module code keeps the old semantics. [1][2]
What kinds of loops
- The change affects for loops (including for-range). The release also documents other related loop features added in 1.22 (e.g., ranging over integers). [2][3]
Compatibility & tooling
- The change was introduced with transition tooling (compiler modes / checks) to help find loops that would behave differently when you opt in. See the proposal and tooling notes for migration guidance. [1][2]
Relevant sources
- Proposal / discussion (golang/go Issue #60078). [1]
- “Fixing For Loops in Go 1.22” (Go blog). [2]
- Go 1.22 release notes. [3]
@csrwng You're absolutely right! Go 1.22 introduced per-iteration loop variable scoping, and this project uses Go 1.24.4, so the loop variable capture issue no longer exists. The suggested shadowing (workload := workload) is unnecessary with Go 1.22+.
Introduce a new Ginkgo v2 based end-to-end test suite with improved structure and organization. The new suite includes: - Suite setup with BeforeSuite/AfterSuite hooks for test initialization - Test organization under test/e2e/v2/tests/ directory - Internal utilities and helpers in test/e2e/v2/internal/ - CRD utility functions in test/e2e/util/ for cross-version compatibility This new test framework will enable more maintainable and scalable e2e testing with better parallel execution support and clearer test structure. Signed-off-by: Cesar Wong <[email protected]> Assisted-by: Claude 3.7 Sonnet (via Claude Code)
csrwng
force-pushed
the
v2-tests-on-main
branch
from
cd2d957 to
103d168
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/v2/internal/test_context.go (1)
58-70: Prefer using the propagated test context instead ofcontext.Background().
TestContextalready embeds the caller’s context, but these calls still hardcodecontext.Background(), bypassing cancellation and deadlines. Reusetc.Contextso the client operations respect the suite’s lifecycle (this was flagged earlier and still applies).- err := tc.MgmtClient.Get(context.Background(), crclient.ObjectKey{ + err := tc.MgmtClient.Get(tc.Context, crclient.ObjectKey{ Namespace: tc.ClusterNamespace, Name: tc.ClusterName, }, hostedCluster) @@ - err = e2eutil.SetReleaseVersionFromHostedCluster(context.Background(), hostedCluster) + err = e2eutil.SetReleaseVersionFromHostedCluster(tc.Context, hostedCluster)test/e2e/v2/internal/workload_resolver.go (1)
159-278: Find the controller owner instead of assuming index 0.Both
belongsToWorkloadandResolveWorkloadForPodgrabpod.OwnerReferences[0](and laterrs.OwnerReferences[0],job.OwnerReferences[0]). Pods can carry multiple owner references, with the managing controller marked bycontroller: true. Relying on position means the resolver breaks whenever a non-controller reference is listed first, so workloads will be misidentified or skipped entirely. Please locate the controller entry explicitly (e.g., viametav1.GetControllerOf) at each step before traversing.- owner := pod.OwnerReferences[0] + controllerRef := metav1.GetControllerOf(pod) + if controllerRef == nil { + return false + } + owner := *controllerRef @@ - if len(rs.OwnerReferences) > 0 && rs.OwnerReferences[0].Kind == "Deployment" { - nextOwner = &rs.OwnerReferences[0] + if controller := metav1.GetControllerOf(rs); controller != nil && controller.Kind == "Deployment" { + nextOwner = controller @@ - if len(job.OwnerReferences) > 0 && job.OwnerReferences[0].Kind == "CronJob" { - nextOwner = &job.OwnerReferences[0] + if controller := metav1.GetControllerOf(job); controller != nil && controller.Kind == "CronJob" { + nextOwner = controllerApply the same pattern in
ResolveWorkloadForPodbefore entering the traversal loop. This guarantees we always follow the true managing controller.
🧹 Nitpick comments (2)
test/e2e/v2/internal/env_vars.go (1)
1-1: Update to modern build tag syntax.The old-style build constraint
// +build e2ev2should be updated to//go:build e2ev2(preferred since Go 1.17).Apply this change:
-// +build e2ev2 +//go:build e2ev2test/e2e/v2/internal/workload_registry.go (1)
1-1: Update to modern build tag syntax.The old-style build constraint
// +build e2ev2should be updated to//go:build e2ev2. Since this is generated code (per line 3), update the generator script to emit the modern syntax.Apply this change:
-// +build e2ev2 +//go:build e2ev2
📜 Review details
Configuration used: CodeRabbit 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 selected for processing (15)
Dockerfile.e2e(1 hunks)Makefile(3 hunks)test/e2e/util/crd.go(1 hunks)test/e2e/util/version.go(2 hunks)test/e2e/v2/internal/env_vars.go(1 hunks)test/e2e/v2/internal/test_context.go(1 hunks)test/e2e/v2/internal/workload_registry.go(1 hunks)test/e2e/v2/internal/workload_resolver.go(1 hunks)test/e2e/v2/tests/api_ux_validation_test.go(1 hunks)test/e2e/v2/tests/assets/hostedcluster-base.yaml(1 hunks)test/e2e/v2/tests/assets/karpenter-nodepool.yaml(1 hunks)test/e2e/v2/tests/assets/karpenter-workloads.yaml(1 hunks)test/e2e/v2/tests/assets/nodepool-base.yaml(1 hunks)test/e2e/v2/tests/control_plane_workloads_test.go(1 hunks)test/e2e/v2/tests/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/util/version.go
- test/e2e/v2/tests/suite_test.go
- test/e2e/v2/tests/assets/karpenter-workloads.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:
test/e2e/v2/tests/assets/hostedcluster-base.yamlDockerfile.e2etest/e2e/util/crd.gotest/e2e/v2/tests/assets/karpenter-nodepool.yamltest/e2e/v2/tests/control_plane_workloads_test.gotest/e2e/v2/internal/workload_registry.gotest/e2e/v2/tests/assets/nodepool-base.yamltest/e2e/v2/internal/test_context.gotest/e2e/v2/tests/api_ux_validation_test.gotest/e2e/v2/internal/env_vars.gotest/e2e/v2/internal/workload_resolver.goMakefile
🔇 Additional comments (7)
Dockerfile.e2e (1)
17-17: LGTM!The addition of the v2 test binary follows the existing pattern and integrates cleanly with the multi-stage build.
Makefile (1)
41-41: LGTM!The v2 test target follows the established pattern and integrates cleanly with the existing e2e workflow.
Also applies to: 272-272, 282-286
test/e2e/util/crd.go (1)
15-93: LGTM! Schema traversal implementation is solid.The recursive CRD schema field checker handles common cases well (properties, array items, schema combinators). The documented limitation around
$refresolution is acceptable for the typical use case, but be aware this may not resolve fields defined through external references.test/e2e/v2/internal/env_vars.go (1)
26-130: LGTM! Well-structured environment variable registry.The registry pattern with sorted help output and sensitive value masking is well-designed. The panic behavior in
GetEnvVarValuefor unregistered variables is appropriate for test infrastructure where early detection of configuration errors is valuable.test/e2e/v2/tests/assets/karpenter-nodepool.yaml (1)
1-22: LGTM!The Karpenter NodePool configuration is appropriate for test scenarios with a focused instance type selection.
test/e2e/v2/tests/assets/nodepool-base.yaml (1)
1-26: LGTM!The base NodePool template uses appropriate placeholders and a safe default (
replicas: 0) that prevents unintended resource creation.test/e2e/v2/internal/workload_registry.go (1)
12-410: LGTM! Comprehensive control plane workload registry.The workload specifications provide good coverage of control plane components with platform-specific scoping where needed.
| persistentVolume: | ||
| size: "8Gi" | ||
| dns: | ||
| baseDomain: agarcial.hypershift.devcluster.openshift.com |
There was a problem hiding this comment.
Replace specific domain with a more generic placeholder.
The base template uses agarcial.hypershift.devcluster.openshift.com, which appears to be a user-specific domain. Base templates should use more generic placeholders (e.g., example.com or clearly indicate these values must be replaced) to avoid confusion when other developers or teams use this template.
Consider using a pattern like:
baseDomain: example.hypershift.localOr add a comment indicating this is a placeholder that must be customized.
Also applies to: 34-34, 39-39, 44-44, 49-49
🤖 Prompt for AI Agents
In test/e2e/v2/tests/assets/hostedcluster-base.yaml around lines 15 (and
similarly at lines 34, 39, 44, 49), the file contains a user-specific domain
value "agarcial.hypershift.devcluster.openshift.com"; replace these with a
generic placeholder (for example "example.hypershift.local" or "example.com") or
add an inline comment indicating the value must be customized, ensuring all
occurrences at the listed lines are updated consistently.
|
/lgtm |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@csrwng: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@csrwng: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What this PR does / why we need it:
Introduce a new Ginkgo v2 based end-to-end test suite with improved
structure and organization. The new suite includes:
This new test framework will enable more maintainable and scalable e2e
testing with better parallel execution support and clearer test structure.
Signed-off-by: Cesar Wong [email protected]
Assisted-by: Claude 3.7 Sonnet (via Claude Code)
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-1857
Checklist: