e2e: fix tuned deferred tests on Hypershift#1494
e2e: fix tuned deferred tests on Hypershift#1494SargunNarula wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors a test to replace Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/operator-utils@v1.4.13: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20191104093116-d3cd4ed1dbcf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition@v0.35.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/v2@v2.26.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/go-units@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/stdr@v1.2.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0 ... [truncated 19339 characters] ... is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/legacy-cloud-providers: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/performanceprofile/functests/2_performance_update/tuned_deferred.go (1)
29-30: Minor: import ordering.Alphabetically,
podsprecedespoolname, so goimports/gofmt grouping would placepodsbeforepoolname. Not flagged by static analysis here, so optional — feel free to swap to match the canonical ordering used elsewhere in this package.♻️ Proposed tweak
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/nodes" - "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/poolname" "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/pods" + "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/poolname"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/performanceprofile/functests/2_performance_update/tuned_deferred.go` around lines 29 - 30, The import block in tuned_deferred.go has the two local imports out of alphabetical order; swap the two import lines so that "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/pods" comes before "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/poolname" to match goimports/gofmt canonical ordering and the package convention used elsewhere (no functional changes, just reorder the imports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/e2e/performanceprofile/functests/2_performance_update/tuned_deferred.go`:
- Around line 29-30: The import block in tuned_deferred.go has the two local
imports out of alphabetical order; swap the two import lines so that
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/pods"
comes before
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/poolname"
to match goimports/gofmt canonical ordering and the package convention used
elsewhere (no functional changes, just reorder the imports).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81fb6235-166a-4620-bf50-bc3e6b4494af
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/2_performance_update/tuned_deferred.go
8b2194c to
66fb75f
Compare
| poolName, err = mcps.GetByProfile(profile) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| testlog.Infof("using performanceMCP: %q", poolName) | ||
| poolName = poolname.GetByProfile(context.TODO(), profile) |
There was a problem hiding this comment.
better to rename the import (utilpoolname?) because poolname vs poolName is too similar and too confusing. We have a cheap fix, let's use it.
other than that LGTM
Signed-off-by: Sargun Narula <snarula@redhat.com>
66fb75f to
f9e8557
Compare
|
/approve thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani, SargunNarula The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@SargunNarula: 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. |
The tuned-deferred suite used to failed in
BeforeAllwhen resolving the pool viamcps.GetByProfile.This PR updates the performance profile test pool discovery logic to use
poolname.GetByProfileinstead, which maps to MCP on self-managed OCP and to the NodePool on Hypershift.Summary by CodeRabbit