OCPCLOUD-3044: Add OpenShift Test Extension (OTE) framework integration#294
Open
sunzhaohua2 wants to merge 2 commits intoopenshift:mainfrom
Open
OCPCLOUD-3044: Add OpenShift Test Extension (OTE) framework integration#294sunzhaohua2 wants to merge 2 commits intoopenshift:mainfrom
sunzhaohua2 wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@sunzhaohua2: This pull request references OCPCLOUD-3044 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds an OpenShift tests extension: new Go module and CLI that builds/registers test suites with platform filters, placeholder e2e specs, Makefile target to build the extension, and Dockerfiles updated to include a gzipped extension binary and updated image labels/tags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Makefile
participant Builder
participant Image as Container Image
participant CLI as openshift-tests-extension CLI
participant Ginkgo as Ginkgo Spec Builder
participant Registry as Extension Registry
participant Runner as Test Runner
Makefile->>Builder: go build openshift-tests-extension -> bin/cluster-machine-approver-ext
Builder->>Builder: gzip bin/cluster-machine-approver-ext -> cluster-machine-approver-ext.gz
Builder->>Image: COPY cluster-machine-approver-ext.gz -> /usr/bin/
Runner->>CLI: invoke extension commands
CLI->>Ginkgo: request generated specs
Ginkgo->>CLI: return specs (with [platform:*]/[NoPlatform:*] labels)
CLI->>Registry: apply PlatformEquals filters, register suites
Registry->>Runner: expose suites/commands for execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Contributor
|
@sunzhaohua2: This pull request references OCPCLOUD-3044 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
openshift-tests-extension/cmd/main.go (3)
76-80: Simplify command execution.The anonymous function wrapper around
root.Execute()is unnecessary.Proposed simplification
- if err := func() error { - return root.Execute() - }(); err != nil { + if err := root.Execute(); err != nil { os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 76 - 80, The anonymous wrapper around root.Execute() is unnecessary; replace the anonymous func invocation with a direct call to root.Execute() and handle its error return directly (i.e., if err := root.Execute(); err != nil { os.Exit(1) }). Update the code that currently wraps root.Execute() in an inline func so it uses root.Execute() directly and preserves the same error-path behavior.
53-65: Move regex compilation outside the loop.The regex is compiled on every iteration of the outer loop, which is inefficient. Additionally, the pattern
[a-z]*only matches lowercase letters.Proposed optimization
+ // Pre-compile regex for platform label extraction + platformRe := regexp.MustCompile(`\[platform:([a-zA-Z0-9]+)]`) + for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { - re := regexp.MustCompile(`\[platform:[a-z]*]`) - - match := re.FindStringSubmatch(test) + match := platformRe.FindStringSubmatch(test) for _, platformDef := range match { if _, ok := foundPlatforms[platformDef]; !ok { platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 53 - 65, Move regex compilation out of the loop and broaden the character class to include uppercase letters; create the regexp once (e.g., re := regexp.MustCompile(`[platform:[A-Za-z]+]`) or similar) before iterating over specs.Select(...).Names(), then use that compiled re inside the loop to find matches for platformDef, and keep the existing logic that extracts the platform via strings.Index and updates foundPlatforms and the specs.Select(...).Include(...) calls.
56-63: Platform extraction logic can be simplified using capture groups.The current approach manually extracts the platform name using
strings.Index. Since you're already using regex, a capture group would be cleaner and less error-prone.Proposed simplification using capture groups
- for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { - re := regexp.MustCompile(`\[platform:[a-z]*]`) - - match := re.FindStringSubmatch(test) - for _, platformDef := range match { - if _, ok := foundPlatforms[platformDef]; !ok { - platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1] - foundPlatforms[platformDef] = platform - specs.Select(extensiontests.NameContains(platformDef)). - Include(extensiontests.PlatformEquals(platform)) - } - } - } + platformRe := regexp.MustCompile(`\[platform:([a-zA-Z0-9]+)]`) + for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { + matches := platformRe.FindAllStringSubmatch(test, -1) + for _, match := range matches { + if len(match) < 2 { + continue + } + platformDef, platform := match[0], match[1] + if _, ok := foundPlatforms[platformDef]; !ok { + foundPlatforms[platformDef] = platform + specs.Select(extensiontests.NameContains(platformDef)). + Include(extensiontests.PlatformEquals(platform)) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 56 - 63, The platform extraction manually slices platformDef after using re.FindStringSubmatch; change the regex to include a capture group for the platform and use the captured value (e.g., match[1]) instead of computing substrings via strings.Index and slicing, update the loop over match/match[i] to read the capture into a variable (platform) and keep the existing deduping in foundPlatforms and the specs.Select(...).Include(...) calls (references: re.FindStringSubmatch, match, platformDef, foundPlatforms, specs.Select, extensiontests.NameContains, extensiontests.PlatformEquals).Makefile (1)
69-71: Ensurebin/directory exists before building.The
go buildcommand will fail if thebin/directory doesn't exist. Consider adding directory creation:Proposed fix
tests-ext: ## Build OpenShift Tests Extension binary - cd openshift-tests-extension && go build -mod=vendor -o ../bin/cluster-machine-approver-ext ./cmd + mkdir -p bin + cd openshift-tests-extension && go build -mod=vendor -o ../bin/cluster-machine-approver-ext ./cmd .PHONY: tests-ext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 69 - 71, The tests-ext Makefile target runs go build outputting to ../bin/cluster-machine-approver-ext but doesn't ensure ../bin exists; update the tests-ext target (label: tests-ext) to create the bin directory before running go build (e.g., mkdir -p ../bin) so the go build -o ../bin/cluster-machine-approver-ext ./cmd succeeds even when bin/ is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 1-4: The Dockerfile's base image uses "golang-1.22" which is
inconsistent with Dockerfile.rhel ("golang-1.24") and the module requirement in
openshift-tests-extension/go.mod ("go 1.25.0"); update the FROM line in this
Dockerfile to use a Go image matching the minimum required by go.mod (e.g.,
change "golang-1.22" to a matching "golang-1.25" or later tag) and ensure
Dockerfile.rhel is updated to the same Go version so all build images
(referencing the FROM line) align with the go.mod requirement.
In `@openshift-tests-extension/go.mod`:
- Line 3: go.mod declares Go 1.25.0 but the build images in the Dockerfiles are
older; update the two builder image references in Dockerfile and Dockerfile.rhel
to use a Go 1.25-compatible image (replace golang-1.22 and golang-1.24-openshift
tags with an official or OpenShift-compatible Go 1.25 tag) so the builder
runtime matches go.mod, or alternatively change the go directive in go.mod back
to the older version if you cannot update the images.
---
Nitpick comments:
In `@Makefile`:
- Around line 69-71: The tests-ext Makefile target runs go build outputting to
../bin/cluster-machine-approver-ext but doesn't ensure ../bin exists; update the
tests-ext target (label: tests-ext) to create the bin directory before running
go build (e.g., mkdir -p ../bin) so the go build -o
../bin/cluster-machine-approver-ext ./cmd succeeds even when bin/ is missing.
In `@openshift-tests-extension/cmd/main.go`:
- Around line 76-80: The anonymous wrapper around root.Execute() is unnecessary;
replace the anonymous func invocation with a direct call to root.Execute() and
handle its error return directly (i.e., if err := root.Execute(); err != nil {
os.Exit(1) }). Update the code that currently wraps root.Execute() in an inline
func so it uses root.Execute() directly and preserves the same error-path
behavior.
- Around line 53-65: Move regex compilation out of the loop and broaden the
character class to include uppercase letters; create the regexp once (e.g., re
:= regexp.MustCompile(`[platform:[A-Za-z]+]`) or similar) before iterating over
specs.Select(...).Names(), then use that compiled re inside the loop to find
matches for platformDef, and keep the existing logic that extracts the platform
via strings.Index and updates foundPlatforms and the
specs.Select(...).Include(...) calls.
- Around line 56-63: The platform extraction manually slices platformDef after
using re.FindStringSubmatch; change the regex to include a capture group for the
platform and use the captured value (e.g., match[1]) instead of computing
substrings via strings.Index and slicing, update the loop over match/match[i] to
read the capture into a variable (platform) and keep the existing deduping in
foundPlatforms and the specs.Select(...).Include(...) calls (references:
re.FindStringSubmatch, match, platformDef, foundPlatforms, specs.Select,
extensiontests.NameContains, extensiontests.PlatformEquals).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 435d1da8-4e48-4f0f-920f-39abe2980bdd
⛔ Files ignored due to path filters (294)
openshift-tests-extension/go.sumis excluded by!**/*.sumopenshift-tests-extension/vendor/cel.dev/expr/.bazelversionis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/.gitattributesis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/.gitignoreis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/CODE_OF_CONDUCT.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/CONTRIBUTING.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/GOVERNANCE.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/MAINTAINERS.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/MODULE.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/README.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/WORKSPACEis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/WORKSPACE.bzlmodis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/checked.pb.gois excluded by!**/*.pb.go,!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/cloudbuild.yamlis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/eval.pb.gois excluded by!**/*.pb.go,!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/explain.pb.gois excluded by!**/*.pb.go,!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/regen_go_proto.shis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/regen_go_proto_canonical_protos.shis excluded by!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/syntax.pb.gois excluded by!**/*.pb.go,!**/vendor/**openshift-tests-extension/vendor/cel.dev/expr/value.pb.gois excluded by!**/*.pb.go,!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/.gitignoreis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/README.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/antlrdoc.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_config.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_config_set.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_deserialization_options.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_deserializer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_simulator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_state.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/atn_type.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/char_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/common_token_factory.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/common_token_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/comparators.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/configuration.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/dfa.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/dfa_serializer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/dfa_state.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/diagnostic_error_listener.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/error_listener.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/error_strategy.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/errors.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/file_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/input_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/int_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/interval_set.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/jcollect.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/lexer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/lexer_action.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/lexer_action_executor.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/lexer_atn_simulator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/ll1_analyzer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/nostatistics.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/parser.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/parser_atn_simulator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/parser_rule_context.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/prediction_context.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/prediction_context_cache.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/prediction_mode.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/recognizer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/rule_context.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/semantic_context.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/statistics.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/stats_data.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/token.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/token_source.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/token_stream.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/tokenstream_rewriter.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/trace_listener.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/transition.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/tree.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/trees.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/antlr4-go/antlr/v4/utils.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/CHANGELOG.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/CONTRIBUTING.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/README.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/SECURITY.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/context.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/context_noslog.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/context_slog.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/discard.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/funcr/slogsink.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/logr.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/sloghandler.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/slogr.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-logr/logr/slogsink.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/.editorconfigis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/.gitattributesis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/.gitignoreis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/CHANGELOG.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/LICENSE.txtis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/README.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/Taskfile.ymlis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/crypto.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/date.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/defaults.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/dict.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/doc.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/functions.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/list.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/network.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/numeric.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/reflect.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/regex.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/strings.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/go-task/slim-sprig/v3/url.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/cel.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/decls.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/env.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/folding.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/inlining.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/io.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/library.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/macro.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/optimizer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/options.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/program.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/prompt.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/templates/authoring.tmplis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/cel/validator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/checker.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/cost.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/decls/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/decls/decls.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/env.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/errors.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/format.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/mapping.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/options.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/printer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/scopes.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/checker/types.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/ast.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/conversion.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/expr.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/factory.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/ast/navigable.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/containers/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/containers/container.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/cost.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/debug/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/debug/debug.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/decls/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/decls/decls.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/doc.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/env/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/env/env.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/error.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/errors.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/functions/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/functions/functions.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/location.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/operators/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/operators/operators.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/overloads/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/overloads/overloads.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/runes/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/runes/buffer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/source.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/stdlib/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/stdlib/standard.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/any_value.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/bool.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/bytes.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/compare.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/doc.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/double.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/duration.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/err.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/format.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/int.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/iterator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/json_value.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/list.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/map.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/null.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/object.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/optional.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/overflow.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/checked.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/enum.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/equal.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/file.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/pb.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/pb/type.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/provider.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/ref/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/ref/provider.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/ref/reference.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/string.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/timestamp.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/comparer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/container.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/field_tester.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/indexer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/iterator.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/lister.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/mapper.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/matcher.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/math.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/receiver.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/sizer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/traits.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/traits/zeroer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/types.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/uint.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/unknown.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/common/types/util.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/activation.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/attribute_patterns.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/attributes.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/decorators.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/dispatcher.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/evalstate.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/interpretable.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/interpreter.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/optimizations.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/planner.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/prune.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/interpreter/runtimecost.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/BUILD.bazelis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/errors.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/BUILD.bazelis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/CEL.g4is excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/CEL.interpis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/CEL.tokensis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/CELLexer.interpis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/CELLexer.tokensis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_base_listener.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_base_visitor.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_lexer.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_listener.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_parser.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/cel_visitor.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/doc.gois excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/gen/generate.shis excluded by!**/gen/**,!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/helper.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/input.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/macro.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/options.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/parser.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/unescape.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/cel-go/parser/unparser.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/compare.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/export.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/diff/debug_disable.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/diff/debug_enable.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/diff/diff.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/flags/flags.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/function/func.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/value/name.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/value/pointer.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/internal/value/sort.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/options.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/path.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_compare.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_references.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_reflect.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_slices.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_text.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/go-cmp/cmp/report_value.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/AUTHORSis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/CONTRIBUTORSis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/encode.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/filter.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/index.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/legacy_java_profile.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/legacy_profile.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/merge.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/profile.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/proto.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/google/pprof/profile/prune.gois excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/inconshreveable/mousetrap/LICENSEis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/inconshreveable/mousetrap/README.mdis excluded by!**/vendor/**openshift-tests-extension/vendor/github.com/inconshreveable/mousetrap/trap_others.gois excluded by!**/vendor/**
📒 Files selected for processing (6)
DockerfileDockerfile.rhelMakefileopenshift-tests-extension/cmd/main.goopenshift-tests-extension/go.modopenshift-tests-extension/test/e2e/machine_approver.go
Contributor
|
@sunzhaohua2: This pull request references OCPCLOUD-3044 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openshift-tests-extension/cmd/main.go (1)
45-48: Replace panic with controlled CLI error handling.In CLI entrypoints, prefer
fmt.Fprintf(os.Stderr, ...)andos.Exit(1)over panic. Additionally, the current format uses%+vbut then calls.Error(), which strips error type information. Use%vto preserve error context:♻️ Suggested error-path adjustment
specs, err := g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() if err != nil { - panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) + fmt.Fprintf(os.Stderr, "couldn't build extension test specs from ginkgo: %v\n", err) + os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 45 - 48, Replace the panic in the CLI entrypoint that follows g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() with controlled error handling: capture the error and write a user-friendly message to stderr using fmt.Fprintf(os.Stderr, "couldn't build extension test specs from ginkgo: %v\n", err) (use %v and pass err directly, do not call .Error() or use %+v), then call os.Exit(1) to terminate with a non-zero status; update the block containing g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() accordingly.openshift-tests-extension/test/e2e/machine_approver.go (1)
25-33: Remove[Conformance]labels from placeholder test specs.These specs are unconditionally skipped and have no test bodies, so the conformance label misrepresents their readiness status.
♻️ Suggested change
- It("should approve the CSR [Conformance]", func() { + It("should approve the CSR", func() { Skip("E2E test placeholder - requires real cluster") }) - It("should reject the CSR [Conformance]", func() { + It("should reject the CSR", func() { Skip("E2E test placeholder - requires real cluster") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/test/e2e/machine_approver.go` around lines 25 - 33, Remove the misleading "[Conformance]" tag from the two placeholder It specs so they no longer claim conformance: update the It descriptions "should approve the CSR [Conformance]" and "should reject the CSR [Conformance]" to remove the bracketed label (e.g., "should approve the CSR" and "should reject the CSR") while leaving the Skip(...) placeholders and surrounding Context block unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift-tests-extension/cmd/main.go`:
- Around line 53-64: The loop that extracts platform labels uses
regexp.MustCompile(`\[platform:[a-z]*]`) and re.FindStringSubmatch(test), which
only returns the first match; change the regex to `\[platform:[a-z0-9-]+\]` and
use re.FindAllString(test, -1) to get all labels, then iterate over each
platformDef string, extract the platform name (same slice logic using
strings.Index and len) and update foundPlatforms and the
specs.Select(...).Include(...) calls (referencing specs.Select,
extensiontests.NameContains, foundPlatforms, platformDef, and
extensiontests.PlatformEquals) so tests with multiple `[platform:*]` labels are
all processed.
---
Nitpick comments:
In `@openshift-tests-extension/cmd/main.go`:
- Around line 45-48: Replace the panic in the CLI entrypoint that follows
g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() with controlled error
handling: capture the error and write a user-friendly message to stderr using
fmt.Fprintf(os.Stderr, "couldn't build extension test specs from ginkgo: %v\n",
err) (use %v and pass err directly, do not call .Error() or use %+v), then call
os.Exit(1) to terminate with a non-zero status; update the block containing
g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() accordingly.
In `@openshift-tests-extension/test/e2e/machine_approver.go`:
- Around line 25-33: Remove the misleading "[Conformance]" tag from the two
placeholder It specs so they no longer claim conformance: update the It
descriptions "should approve the CSR [Conformance]" and "should reject the CSR
[Conformance]" to remove the bracketed label (e.g., "should approve the CSR" and
"should reject the CSR") while leaving the Skip(...) placeholders and
surrounding Context block unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1c1c73b-8aa6-4db6-894d-5fc832f19698
⛔ Files ignored due to path filters (1)
openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
DockerfileDockerfile.rhelMakefileopenshift-tests-extension/cmd/main.goopenshift-tests-extension/go.modopenshift-tests-extension/test/e2e/machine_approver.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- Dockerfile.rhel
- Dockerfile
Comment on lines
+53
to
+64
| for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { | ||
| re := regexp.MustCompile(`\[platform:[a-z]*]`) | ||
|
|
||
| match := re.FindStringSubmatch(test) | ||
| for _, platformDef := range match { | ||
| if _, ok := foundPlatforms[platformDef]; !ok { | ||
| platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1] | ||
| foundPlatforms[platformDef] = platform | ||
| specs.Select(extensiontests.NameContains(platformDef)). | ||
| Include(extensiontests.PlatformEquals(platform)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "main.go" -path "*/openshift-tests-extension/cmd/*" -type fRepository: openshift/cluster-machine-approver
Length of output: 117
🏁 Script executed:
head -100 openshift-tests-extension/cmd/main.go | cat -nRepository: openshift/cluster-machine-approver
Length of output: 3151
🏁 Script executed:
sed -n '40,75p' openshift-tests-extension/cmd/main.go | cat -nRepository: openshift/cluster-machine-approver
Length of output: 1560
🏁 Script executed:
# Search for test files to understand platform label usage
find openshift-tests-extension -name "*.go" -type f | grep test | head -20Repository: openshift/cluster-machine-approver
Length of output: 1504
🏁 Script executed:
# Look for tests that might have multiple [platform:*] labels
rg '\[platform:[^\]]+\].*\[platform:[^\]]+\]' openshift-tests-extension --type=goRepository: openshift/cluster-machine-approver
Length of output: 60
🏁 Script executed:
# Look for platform labels with numbers, uppercase, or hyphens
rg '\[platform:[^\]]*[0-9A-Z_-][^\]]*\]' openshift-tests-extension --type=goRepository: openshift/cluster-machine-approver
Length of output: 164
🏁 Script executed:
# Let's examine the test file to see what platform labels are actually used
cat openshift-tests-extension/test/e2e/machine_approver.go | head -200Repository: openshift/cluster-machine-approver
Length of output: 1119
🏁 Script executed:
# Search more broadly for all [platform:*] occurrences in the repo
rg '\[platform:[^\]]*\]' openshift-tests-extension --type=go -v vendorRepository: openshift/cluster-machine-approver
Length of output: 50394
🏁 Script executed:
# Search for actual test names/strings that might contain platform labels
rg -i 'platform' openshift-tests-extension --type=go -v vendor | grep -v "\.pb\.go"Repository: openshift/cluster-machine-approver
Length of output: 50455
🏁 Script executed:
# Let's check if there are any test specifications or configs that define platform labels
find openshift-tests-extension -name "*.go" -type f ! -path "*/vendor/*" | xargs grep -l platform 2>/dev/nullRepository: openshift/cluster-machine-approver
Length of output: 115
🏁 Script executed:
# Let's understand the actual behavior of FindStringSubmatch vs FindAllString in Go
# by creating a test to show the difference
python3 << 'EOF'
# Simulate Go's regex behavior
import re
# Test case: a string with multiple platform labels
test_name = "some-test-name-[platform:aws][platform:gcp]"
pattern = r'\[platform:[a-z]*\]'
# FindStringSubmatch equivalent (returns first match only in the array)
match_submatch = re.search(pattern, test_name)
if match_submatch:
print("FindStringSubmatch behavior:")
print(f" Match: {match_submatch.group(0)}")
print(f" Iteration would yield: [{match_submatch.group(0)}]")
print()
# FindAllString equivalent (returns all matches)
matches_all = re.findall(pattern, test_name)
print("FindAllString behavior:")
print(f" Matches: {matches_all}")
print(f" Would iterate over: {len(matches_all)} items")
EOFRepository: openshift/cluster-machine-approver
Length of output: 275
Use FindAllString to handle tests with multiple [platform:*] labels.
Line 56 uses FindStringSubmatch, which returns only the first match. This means any test with multiple platform labels (e.g., test-name [platform:aws][platform:gcp]) will only process the first label and skip the rest. Use FindAllString instead to capture all platform labels in a single test name.
Also consider updating the regex pattern from [a-z]* to [a-z0-9-]+ to support platform names with numbers and hyphens.
Suggested fix
- for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() {
- re := regexp.MustCompile(`\[platform:[a-z]*]`)
-
- match := re.FindStringSubmatch(test)
- for _, platformDef := range match {
+ re := regexp.MustCompile(`\[platform:[a-z0-9-]+]`)
+ for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() {
+ matches := re.FindAllString(test, -1)
+ for _, platformDef := range matches {
if _, ok := foundPlatforms[platformDef]; !ok {
platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1]
foundPlatforms[platformDef] = platform
specs.Select(extensiontests.NameContains(platformDef)).
Include(extensiontests.PlatformEquals(platform))
}
}
}📝 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.
Suggested change
| for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { | |
| re := regexp.MustCompile(`\[platform:[a-z]*]`) | |
| match := re.FindStringSubmatch(test) | |
| for _, platformDef := range match { | |
| if _, ok := foundPlatforms[platformDef]; !ok { | |
| platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1] | |
| foundPlatforms[platformDef] = platform | |
| specs.Select(extensiontests.NameContains(platformDef)). | |
| Include(extensiontests.PlatformEquals(platform)) | |
| } | |
| } | |
| re := regexp.MustCompile(`\[platform:[a-z0-9-]+]`) | |
| for _, test := range specs.Select(extensiontests.NameContains("[platform:")).Names() { | |
| matches := re.FindAllString(test, -1) | |
| for _, platformDef := range matches { | |
| if _, ok := foundPlatforms[platformDef]; !ok { | |
| platform := platformDef[strings.Index(platformDef, ":")+1 : len(platformDef)-1] | |
| foundPlatforms[platformDef] = platform | |
| specs.Select(extensiontests.NameContains(platformDef)). | |
| Include(extensiontests.PlatformEquals(platform)) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openshift-tests-extension/cmd/main.go` around lines 53 - 64, The loop that
extracts platform labels uses regexp.MustCompile(`\[platform:[a-z]*]`) and
re.FindStringSubmatch(test), which only returns the first match; change the
regex to `\[platform:[a-z0-9-]+\]` and use re.FindAllString(test, -1) to get all
labels, then iterate over each platformDef string, extract the platform name
(same slice logic using strings.Index and len) and update foundPlatforms and the
specs.Select(...).Include(...) calls (referencing specs.Select,
extensiontests.NameContains, foundPlatforms, platformDef, and
extensiontests.PlatformEquals) so tests with multiple `[platform:*]` labels are
all processed.
Contributor
|
@sunzhaohua2: This pull request references OCPCLOUD-3044 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openshift-tests-extension/cmd/main.go (1)
44-47: Handle spec-discovery failures as normal CLI errors.Panicking here makes the binary exit with a Go stack trace when discovery fails, which is noisy for
info,list, andrun-testcallers. Print the error once and exit non-zero instead. (go.dev)♻️ Suggested change
specs, err := g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() if err != nil { - panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) + fmt.Fprintf(os.Stderr, "couldn't build extension test specs from ginkgo: %v\n", err) + os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 44 - 47, Replace the panic in the call to BuildExtensionTestSpecsFromOpenShiftGinkgoSuite with a normal CLI error exit: when g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() returns an error, print a concise error message (including err.Error()) to stderr (or use the existing logger) and call os.Exit(1) instead of panicking so callers like info/list/run-test don't get a stack trace; update the block around the specs, err := g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() line in main (or the enclosing function) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openshift-tests-extension/cmd/main.go`:
- Around line 44-47: Replace the panic in the call to
BuildExtensionTestSpecsFromOpenShiftGinkgoSuite with a normal CLI error exit:
when g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() returns an error, print
a concise error message (including err.Error()) to stderr (or use the existing
logger) and call os.Exit(1) instead of panicking so callers like
info/list/run-test don't get a stack trace; update the block around the specs,
err := g.BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() line in main (or the
enclosing function) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42fd9917-512b-40e6-bebd-375ad1d24b55
⛔ Files ignored due to path filters (1)
openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
DockerfileDockerfile.rhelMakefileopenshift-tests-extension/cmd/main.goopenshift-tests-extension/go.modopenshift-tests-extension/test/e2e/machine_approver.go
✅ Files skipped from review due to trivial changes (1)
- openshift-tests-extension/test/e2e/machine_approver.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- Dockerfile
- openshift-tests-extension/go.mod
Contributor
|
@sunzhaohua2: An error was encountered searching for bug OCPCLOUD-3044 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
request failed. Please analyze the request body for more details. Status code: 403:
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
openshift-ci-robot
removed
the
jira/valid-reference
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Dockerfile (1)
1-1:⚠️ Potential issue | 🟡 MinorGo version mismatch: base image uses golang-1.22, go.mod requires 1.24.
This inconsistency was previously flagged. Ensure the builder image meets the minimum Go version required by the module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 1, The Dockerfile's builder base image currently uses golang-1.22 (FROM registry.ci.openshift.org/openshift/release:golang-1.22 AS builder) which conflicts with go.mod requiring Go 1.24; update the FROM image to a Go 1.24 (or later) compatible tag—e.g., change the image tag to golang-1.24 or a >=1.24 variant—so the build environment matches the module's go version.openshift-tests-extension/go.mod (1)
3-3:⚠️ Potential issue | 🟡 MinorGo version mismatch with Dockerfile base image.
The module specifies
go 1.24.0, but the Dockerfile usesgolang-1.22. While Go is generally backward compatible, features or dependencies requiring Go 1.24 may fail at build time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/go.mod` at line 3, The module's Go version declaration (go.mod: "go 1.24.0") does not match the Dockerfile base image ("golang-1.22"); update either the go.mod to "go 1.22" or change the Dockerfile base image to a Go 1.24 image (e.g., golang:1.24) so both specify the same Go version, then rebuild to verify compatibility; reference the go.mod "go 1.24.0" line and the Dockerfile base image string "golang-1.22" when making the change.
🧹 Nitpick comments (1)
openshift-tests-extension/cmd/main.go (1)
61-65: Minor: Unnecessary IIFE wrapper aroundroot.Execute().The immediately-invoked function expression adds no value here.
Simplified version
- if err := func() error { - return root.Execute() - }(); err != nil { + if err := root.Execute(); err != nil { os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests-extension/cmd/main.go` around lines 61 - 65, The code wraps root.Execute() in an unnecessary immediately-invoked function expression; replace the IIFE with a direct call to root.Execute(), e.g., capture its returned error into err (err := root.Execute() or if err := root.Execute(); err != nil) and call os.Exit(1) on error; update the block that currently references the IIFE to use a direct invocation of root.Execute() instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Dockerfile`:
- Line 1: The Dockerfile's builder base image currently uses golang-1.22 (FROM
registry.ci.openshift.org/openshift/release:golang-1.22 AS builder) which
conflicts with go.mod requiring Go 1.24; update the FROM image to a Go 1.24 (or
later) compatible tag—e.g., change the image tag to golang-1.24 or a >=1.24
variant—so the build environment matches the module's go version.
In `@openshift-tests-extension/go.mod`:
- Line 3: The module's Go version declaration (go.mod: "go 1.24.0") does not
match the Dockerfile base image ("golang-1.22"); update either the go.mod to "go
1.22" or change the Dockerfile base image to a Go 1.24 image (e.g., golang:1.24)
so both specify the same Go version, then rebuild to verify compatibility;
reference the go.mod "go 1.24.0" line and the Dockerfile base image string
"golang-1.22" when making the change.
---
Nitpick comments:
In `@openshift-tests-extension/cmd/main.go`:
- Around line 61-65: The code wraps root.Execute() in an unnecessary
immediately-invoked function expression; replace the IIFE with a direct call to
root.Execute(), e.g., capture its returned error into err (err := root.Execute()
or if err := root.Execute(); err != nil) and call os.Exit(1) on error; update
the block that currently references the IIFE to use a direct invocation of
root.Execute() instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c93df595-8e85-4b8c-a5e2-0ed9625cb812
⛔ Files ignored due to path filters (1)
openshift-tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
DockerfileDockerfile.rhelMakefileopenshift-tests-extension/cmd/main.goopenshift-tests-extension/go.modopenshift-tests-extension/test/e2e/machine_approver.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- Dockerfile.rhel
Contributor
|
@sunzhaohua2: This PR was included in a payload test run from openshift/origin#30884
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cf468f20-20ea-11f1-985c-856aadf54291-0 |
Contributor
|
@sunzhaohua2: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Author
|
Tested in pr openshift/origin#30884 (comment) From job log we can see |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces the OpenShift test extension (OTE) binary to this project, it will integrate this repo's E2E tests into the OpenShift origin test framework.
For more information regarding the extension, see openshift-eng/openshift-tests-extension repository or the respective enhancement.
Local Tests: