-
Notifications
You must be signed in to change notification settings - Fork 425
CNTRLPLANE-2167:Integrate OpenShift Test Extension (OTE) into oc #2160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@gangwgr: This pull request references CNTRLPLANE-2167 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis pull request introduces a new CLI binary named Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
|
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/oc-tests-ext/main.go (1)
23-45: Propagatecontext.Contextinto the Cobra commandYou construct the root command with a
ctxparameter but never use it, so subcommands can’t benefit from cancellation/timeouts. You can wire it through to Cobra with minimal change:func newOperatorTestCommand(ctx context.Context) *cobra.Command { registry := prepareOperatorTestsRegistry() cmd := &cobra.Command{ Use: "oc-tests-ext", Short: "A binary used to run oc tests as part of OTE.", Run: func(cmd *cobra.Command, args []string) { if err := cmd.Help(); err != nil { klog.Fatal(err) } }, } + + cmd.SetContext(ctx) if v := version.Get().String(); len(v) == 0 { cmd.Version = "<unknown>" } else { cmd.Version = v }This keeps the current behavior while making it easier for extension commands to honor cancellation.
images/cli/Dockerfile.rhel (1)
4-6: Confirm that CI consumers expect a gzippedoc-tests-extin the imageThe builder gzips
oc-tests-extand the final image only contains/usr/bin/oc-tests-ext.gz. That’s fine if downstream CI tooling knows to decompress it before execution; otherwise, callers may be surprised thatoc-tests-extisn’t directly runnable in the image.If the intent is direct invocation inside the image, consider either:
- Dropping the
gzipstep and copying the raw binary, or- Adding a build/entrypoint step that uncompresses
oc-tests-ext.gztooc-tests-extbefore use.Please verify this matches how OTE consumers expect to discover and run the binary.
Also applies to: 10-10
📜 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 (294)
go.sumis excluded by!**/*.sumvendor/cel.dev/expr/.bazelversionis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/.gitattributesis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/CODE_OF_CONDUCT.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/GOVERNANCE.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/MAINTAINERS.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/MODULE.bazelis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/README.mdis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/WORKSPACEis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/WORKSPACE.bzlmodis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/checked.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/cloudbuild.yamlis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/eval.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/explain.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/regen_go_proto.shis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/regen_go_proto_canonical_protos.shis excluded by!vendor/**,!**/vendor/**vendor/cel.dev/expr/syntax.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/cel.dev/expr/value.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/antlrdoc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_config_set.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_deserialization_options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_deserializer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_simulator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_state.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/atn_type.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/char_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/common_token_factory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/common_token_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/comparators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/configuration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/dfa.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/dfa_serializer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/dfa_state.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/diagnostic_error_listener.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/error_listener.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/error_strategy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/file_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/input_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/int_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/interval_set.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/jcollect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/lexer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/lexer_action.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/lexer_action_executor.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/lexer_atn_simulator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/ll1_analyzer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/nostatistics.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/parser.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/parser_atn_simulator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/parser_rule_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/prediction_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/prediction_context_cache.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/prediction_mode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/recognizer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/rule_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/semantic_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/statistics.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/stats_data.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/token.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/token_source.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/token_stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/tokenstream_rewriter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/trace_listener.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/transition.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/tree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/trees.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/antlr4-go/antlr/v4/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/cel.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/decls.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/env.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/folding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/inlining.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/io.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/library.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/macro.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/optimizer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/program.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/prompt.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/templates/authoring.tmplis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/cel/validator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/checker.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/cost.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/decls/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/decls/decls.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/env.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/mapping.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/printer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/scopes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/checker/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/ast.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/conversion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/expr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/factory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/ast/navigable.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/containers/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/containers/container.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/cost.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/debug/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/debug/debug.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/decls/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/decls/decls.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/env/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/env/env.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/functions/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/functions/functions.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/location.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/operators/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/operators/operators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/overloads/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/overloads/overloads.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/runes/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/runes/buffer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/source.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/stdlib/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/stdlib/standard.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/any_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/bool.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/bytes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/compare.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/double.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/duration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/err.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/int.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/iterator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/json_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/list.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/map.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/null.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/object.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/optional.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/overflow.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/checked.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/enum.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/equal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/pb.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/pb/type.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/provider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/ref/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/ref/provider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/ref/reference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/string.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/timestamp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/comparer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/container.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/field_tester.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/indexer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/iterator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/lister.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/mapper.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/math.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/receiver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/sizer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/traits.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/traits/zeroer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/uint.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/unknown.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/common/types/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/activation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/attribute_patterns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/attributes.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/decorators.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/dispatcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/evalstate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/interpretable.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/interpreter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/optimizations.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/planner.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/prune.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/interpreter/runtimecost.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/BUILD.bazelis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/BUILD.bazelis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/CEL.g4is excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/CEL.interpis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/CEL.tokensis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/CELLexer.interpis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/CELLexer.tokensis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_base_listener.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_base_visitor.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_lexer.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_listener.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_parser.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/cel_visitor.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/doc.gois excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/gen/generate.shis excluded by!**/gen/**,!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/helper.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/input.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/macro.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/parser.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/unescape.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/cel-go/parser/unparser.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdimages/cmdimages.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdinfo/info.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdlist/list.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runtest.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdupdate/update.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/dbtime/time.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extension.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/environment.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/spec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/task.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/registry.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/component.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/concurrency.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/environment.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/names.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/output.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/flags/suite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/junit/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/byte.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/empty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int32.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/int64.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/set.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/util/sets/string.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/version/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/.golangci.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/camel.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/helper.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/kebab.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/stoewer/go-strcase/snake.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/constraints/constraints.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/slices/cmp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/slices/slices.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/slices/sort.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/slices/zsortanyfunc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/exp/slices/zsortordered.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/checked.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/eval.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/explain.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/syntax.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/api/expr/v1alpha1/value.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/LICENSEis 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/protobuf/encoding/protojson/decode.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protojson/doc.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protojson/encode.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/editionssupport/editions.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/encoding/json/decode_number.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/encoding/json/decode_string.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/encoding/json/decode_token.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/internal/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)cmd/oc-tests-ext/main.go(1 hunks)go.mod(5 hunks)images/cli/Dockerfile.rhel(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:
README.mdcmd/oc-tests-ext/main.goMakefileimages/cli/Dockerfile.rhelgo.mod
🧬 Code graph analysis (1)
cmd/oc-tests-ext/main.go (1)
pkg/cli/version/version.go (1)
Version(28-32)
🔇 Additional comments (4)
.gitignore (1)
2-2: Ignore rule for oc-tests-ext binary looks correctAdding
/oc-tests-extkeeps the built test extension binary out of version control and is consistent with other ignored build artifacts here.Makefile (1)
74-77: oc-tests-ext target wiring is consistent with existing build patternThe new
oc-tests-exttarget mirrors the existingoctarget pattern (target-specificGO_BUILD_PACKAGESand flags, then delegating tobuild), which should integrate cleanly into the current build machinery.go.mod (1)
41-41: New OTE-related dependencies look reasonable; verify tidy/vendor and version compatibilityThe added direct dependency on
github.com/openshift-eng/openshift-tests-extensionand its CEL/genproto/ANTLR/strcase indirects are consistent with wiring in an expression‑driven test extension. From the module file alone there’s no obvious issue, but please double‑check that:
go mod tidyand the vendored tree (if applicable) are updated in this PR.- The chosen versions of these libs are compatible with the Go toolchain and other existing dependencies in this repo.
If CI is already green on dependency resolution, this should be fine.
Also applies to: 72-73, 78-78, 139-139, 191-191, 205-206
README.md (1)
64-94: Tests section clearly documents oc-tests-ext usageThe new Tests section gives concise, practical examples for building, running, and listing tests with
oc-tests-ext, and notes image inclusion for CI/CD. This should make the new extension much easier to adopt.
ardaguclu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even for reviewing this PR requires dedicated time and plan which I don't have. So we need to defer this until we plan this change in oc accordingly. Because I don't know the motivations and reasoning behind this change yet.
So I have to add hold for this, until there is someone willing to review this in-detail
/hold
images/cli/Dockerfile.rhel
Outdated
| COPY . . | ||
| RUN make build --warn-undefined-variables | ||
| RUN make build --warn-undefined-variables \ | ||
| && make oc-tests-ext \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many people, CI systems rely on this cli image. This change increases the storage size which may have unexpected impacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ardaguclu We have same discussed with @p0lyn0mial here https://redhat-internal.slack.com/archives/CC3CZCQHM/p1762421042028719 regarding kas-o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR integrates with https://docs.google.com/document/d/1cFZj9QdzW8hbHc3H0Nce-2xrJMtpDJrwAse9H7hLiWk/edit?tab=t.0#heading=h.66y4kqbj468a
TLDR: it’s about integrating tests from this repository with openshift-tests to improve coverage and protect against regressions.
BTW: do we actually have any e2e tests defined for this repository? If not, maybe this PR isn’t needed at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@p0lyn0mial In qe repo we have, need to migrate those here
@zhouying7780 Please confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@p0lyn0mial as checked with @zhouying7780 We have cases here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gangwgr I built this binary on my local. Its size is 56 mb (zipped size is 24 mb). That means this change statically increases every image that relies on cli image with almost 30%. Additionally, this binary will be only used by our CI system so there is no point of building and exposing this to customers.
I think, we can add a separate Makefile target (e.g. make oc-tests-ext) in oc and build this binary separately only in our CI system (https://github.com/openshift/release/blob/104543c7c4e0bc5ad2b60e6d452b266233313efc/ci-operator/config/openshift/oc/openshift-oc-main.yaml#L30). WDYT?.
e6299a3 to
34ac8c5
Compare
|
/test e2e-agnostic-ovn-cmd |
1 similar comment
|
/test e2e-agnostic-ovn-cmd |
ardaguclu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are merge conflicts. So this PR needs to be updated. I dropped some comments.
images/cli/Dockerfile.rhel
Outdated
| COPY . . | ||
| RUN make build --warn-undefined-variables | ||
| RUN make build --warn-undefined-variables \ | ||
| && make oc-tests-ext \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gangwgr I built this binary on my local. Its size is 56 mb (zipped size is 24 mb). That means this change statically increases every image that relies on cli image with almost 30%. Additionally, this binary will be only used by our CI system so there is no point of building and exposing this to customers.
I think, we can add a separate Makefile target (e.g. make oc-tests-ext) in oc and build this binary separately only in our CI system (https://github.com/openshift/release/blob/104543c7c4e0bc5ad2b60e6d452b266233313efc/ci-operator/config/openshift/oc/openshift-oc-main.yaml#L30). WDYT?.
| @@ -0,0 +1,53 @@ | |||
| package main | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in this file make sense to me.
Makefile
Outdated
|
|
||
| oc-tests-ext: GO_BUILD_PACKAGES :=./cmd/oc-tests-ext | ||
| oc-tests-ext: GO_BUILD_FLAGS :=-tags 'include_gcs include_oss containers_image_openpgp' | ||
| oc-tests-ext: build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a separate build target independent from main build and should be executed separately when it is required.
Makefile
Outdated
| .PHONY: oc | ||
|
|
||
| oc-tests-ext: GO_BUILD_PACKAGES :=./cmd/oc-tests-ext | ||
| oc-tests-ext: GO_BUILD_FLAGS :=-tags 'include_gcs include_oss containers_image_openpgp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use these flags for oc-tests-ext binary.
- Remove oc-tests-ext from main CLI image (Dockerfile.rhel) to avoid increasing image size by 30% - Make oc-tests-ext an independent build target in Makefile - Remove unnecessary build flags from oc-tests-ext target - oc-tests-ext will be built separately in CI only when required This addresses feedback from @ardaguclu on making oc-tests-ext a separate build that doesn't bloat the main CLI image.
|
@ardaguclu Regarding this #2160 (comment) we need Dockerfile changes and ote is independent from oc binary. In ci ote binary extracted and run test cases. We can't build ote binary in ci job as ote team asked us not to build ote binary in jobs. |
|
/close |
|
@gangwgr: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
As per discussion https://redhat-internal.slack.com/archives/CC3CZCQHM/p1762421042028719
CNTRLPLANE-2167:Integrate OpenShift Test Extension (OTE) into oc