NO-JIRA: Bump ghw dependency#1492
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds context propagation to snapshot operations, upgrades ghw to v0.23.0, enhances GHWHandler to accept and unpack .tgz/.tar.gz snapshots with temp-dir cleanup, and introduces deferred Cleanup() calls across command code and tests, reporting cleanup errors as warnings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.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" Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmencak 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 |
openshift-ci
Bot
added
the
approved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/performanceprofile/profilecreator/cmd/info.go (1)
40-52:⚠️ Potential issue | 🟠 Major
makeClusterDataerrors can still leak unpacked snapshots.This defer only runs after
makeClusterDatasucceeds. In info mode, a later pool failure can discard handlers already stored for earlier pools without callingCleanup(), so the temp dirs introduced by archive unpacking are leaked. Please add cleanup insidemakeClusterDatabefore returning any error after handlers have been appended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/performanceprofile/profilecreator/cmd/info.go` around lines 40 - 52, The makeClusterData function can append handlers (temp-dir resources) then return an error without cleaning them up; modify makeClusterData so that whenever it returns a non-nil error after any handlers have been appended to the clusterData slice/map it iterates those already-created handlers and calls their Cleanup() (handling/logging any cleanup errors) before returning the error, ensuring no unpacked snapshots remain; reference the makeClusterData function and the handler.Cleanup() method used in the deferred cleanup of clusterData in info.go.pkg/performanceprofile/profilecreator/cmd/root.go (1)
158-168:⚠️ Potential issue | 🟠 MajorCleanup still misses partial
makeNodesHandlersfailures.This defer is registered only after
makeNodesHandlerssucceeds. If one node fails after earlier archive-backed handlers were already created, those handlers' unpacked temp dirs are leaked because the helper drops the partial slice on error.💡 Suggested fix
func makeNodesHandlers(mustGatherDirPath, poolName string, nodes []*corev1.Node) ([]*profilecreator.GHWHandler, error) { - handlers := make([]*profilecreator.GHWHandler, len(nodes)) + handlers := make([]*profilecreator.GHWHandler, 0, len(nodes)) sb := strings.Builder{} - for i, node := range nodes { + for _, node := range nodes { handle, err := profilecreator.NewGHWHandler(mustGatherDirPath, node) if err != nil { + for _, h := range handlers { + _ = h.Cleanup() + } return nil, fmt.Errorf("failed to load node's GHW snapshot: %w", err) } - handlers[i] = handle + handlers = append(handlers, handle) sb.WriteString(node.Name + " ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/performanceprofile/profilecreator/cmd/root.go` around lines 158 - 168, The current code registers the cleanup defer only after makeNodesHandlers returns nil error, which leaks temp dirs when makeNodesHandlers fails after creating some handlers; change the call site so you capture the returned nodesHandlers slice and immediately defer iterating and calling handler.Cleanup() on any non-nil entries before inspecting err (i.e., place the defer right after assigning nodesHandlers, not inside the success branch), and ensure makeNodesHandlers still returns any partially-created handlers on error so the deferred cleanup can run (refer to makeNodesHandlers, nodesHandlers, and handler.Cleanup).
🧹 Nitpick comments (1)
pkg/performanceprofile/profilecreator/profilecreator_test.go (1)
233-235: Add one archive-backedNewGHWHandlertest.These cleanup additions still only exercise directory-backed must-gather input. The new
.tgz/.tar.gzbranch and its failure cleanup inghwhandler.goremain uncovered, so this migration can regress without a test catching it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/performanceprofile/profilecreator/profilecreator_test.go` around lines 233 - 235, Add a test that exercises the archive-backed path of NewGHWHandler by creating a temporary .tgz (or .tar.gz) must-gather archive containing the same test fixture files used by the directory-backed tests, call NewGHWHandler with the archive path and the same node input, assert no error, and ensure DeferCleanup(handle.Cleanup) is used so the failure/cleanup branch in ghwhandler.go is executed; target the NewGHWHandler call site in profilecreator_test.go and verify the handler can open and later Cleanup an archive-backed must-gather to cover the .tgz/.tar.gz branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/performanceprofile/profilecreator/ghwhandler.go`:
- Around line 52-56: When snapshot.Unpack(...) returns an error the temp
directory at tmpDir is left behind; update the error path after calling
snapshot.Unpack in ghwhandler.go to remove the temp dir (e.g., call
os.RemoveAll(tmpDir)) before returning the fmt.Errorf; ensure you only attempt
cleanup when tmpDir is non-empty and preserve the original error returned by
snapshot.Unpack so the function still returns the same error payload.
---
Outside diff comments:
In `@pkg/performanceprofile/profilecreator/cmd/info.go`:
- Around line 40-52: The makeClusterData function can append handlers (temp-dir
resources) then return an error without cleaning them up; modify makeClusterData
so that whenever it returns a non-nil error after any handlers have been
appended to the clusterData slice/map it iterates those already-created handlers
and calls their Cleanup() (handling/logging any cleanup errors) before returning
the error, ensuring no unpacked snapshots remain; reference the makeClusterData
function and the handler.Cleanup() method used in the deferred cleanup of
clusterData in info.go.
In `@pkg/performanceprofile/profilecreator/cmd/root.go`:
- Around line 158-168: The current code registers the cleanup defer only after
makeNodesHandlers returns nil error, which leaks temp dirs when
makeNodesHandlers fails after creating some handlers; change the call site so
you capture the returned nodesHandlers slice and immediately defer iterating and
calling handler.Cleanup() on any non-nil entries before inspecting err (i.e.,
place the defer right after assigning nodesHandlers, not inside the success
branch), and ensure makeNodesHandlers still returns any partially-created
handlers on error so the deferred cleanup can run (refer to makeNodesHandlers,
nodesHandlers, and handler.Cleanup).
---
Nitpick comments:
In `@pkg/performanceprofile/profilecreator/profilecreator_test.go`:
- Around line 233-235: Add a test that exercises the archive-backed path of
NewGHWHandler by creating a temporary .tgz (or .tar.gz) must-gather archive
containing the same test fixture files used by the directory-backed tests, call
NewGHWHandler with the archive path and the same node input, assert no error,
and ensure DeferCleanup(handle.Cleanup) is used so the failure/cleanup branch in
ghwhandler.go is executed; target the NewGHWHandler call site in
profilecreator_test.go and verify the handler can open and later Cleanup an
archive-backed must-gather to cover the .tgz/.tar.gz branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11279e30-b141-438b-9f4d-8e2265abcdbd
⛔ Files ignored due to path filters (79)
go.sumis excluded by!**/*.sumvendor/github.com/jaypipes/ghw/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/SNAPSHOT.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/alias.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/host.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/config/context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/config/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/linuxdmi/dmi_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/linuxpath/path_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/marshal/marshal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_cache_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/option/option.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_block_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_pci_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_usb_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/pack.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/trace.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/unpack.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/util/util.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
cmd/gather-sysinfo/gather-sysinfo.gogo.modpkg/performanceprofile/profilecreator/cmd/info.gopkg/performanceprofile/profilecreator/cmd/root.gopkg/performanceprofile/profilecreator/ghwhandler.gopkg/performanceprofile/profilecreator/profilecreator_test.go
Upgrade ghw library and address API breaking changes. The new ghw API no longer provides the WithSnapshot() function and SnapshotOptions struct that were available in versions prior to v0.21.3. Changes: * Update ghwhandler.go to use new snapshot.Unpack() for archive handling * Replace WithSnapshot() with option.WithChroot() for snapshot paths * Add Cleanup() method to GHWHandler for temporary directory cleanup * Handle both .tgz and .tar.gz archive formats explicitly
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
LGTM with some improvements/questions, please see inline comments
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/performanceprofile/profilecreator/cmd/root.go (1)
158-168:⚠️ Potential issue | 🟠 MajorPartial handler allocation can leak temp dirs on early failure
Cleanup is deferred only after
makeNodesHandlers(...)succeeds. If handler creation fails partway, previously created handlers are not cleaned up.Proposed fix
func makeNodesHandlers(mustGatherDirPath, poolName string, nodes []*corev1.Node) ([]*profilecreator.GHWHandler, error) { handlers := make([]*profilecreator.GHWHandler, len(nodes)) sb := strings.Builder{} for i, node := range nodes { handle, err := profilecreator.NewGHWHandler(mustGatherDirPath, node) if err != nil { + for j := 0; j < i; j++ { + if handlers[j] != nil { + if cleanupErr := handlers[j].Cleanup(); cleanupErr != nil { + Alert("Warning: failed to cleanup handler: %v\n", cleanupErr) + } + } + } return nil, fmt.Errorf("failed to load node's GHW snapshot: %w", err) } handlers[i] = handle sb.WriteString(node.Name + " ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/performanceprofile/profilecreator/cmd/root.go` around lines 158 - 168, The partial allocation of nodesHandlers can leak temp dirs if makeNodesHandlers returns a non-nil slice and an error; update the error path to cleanup any handlers already created before returning: after calling makeNodesHandlers(pcArgs.MustGatherDirPath, pcArgs.NodePoolName, nodes) check if err != nil and if nodesHandlers != nil iterate over each handler and call handler.Cleanup() (log warnings on cleanup errors) before returning err; keep the existing deferred cleanup for the success path (defer loop over nodesHandlers calling Cleanup) so successful paths remain unchanged.
🧹 Nitpick comments (1)
cmd/gather-sysinfo/gather-sysinfo.go (1)
75-75: Preserve error wrapping when returning the clone-content failure.Line 75 should use
%wso callers can unwrap and classify the underlying error.Suggested fix
- return fmt.Errorf("error getting expected clone content: %v", err) + return fmt.Errorf("error getting expected clone content: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gather-sysinfo/gather-sysinfo.go` at line 75, The return statement currently formats the wrapped error with "%v" which prevents callers from unwrapping; change the fmt.Errorf call that returns "error getting expected clone content: ..." to use "%w" for the second verb so the underlying error (err) can be unwrapped by callers (i.e., replace "%v" with "%w" in the fmt.Errorf call that returns when err != nil while getting expected clone content).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gather-sysinfo/gather-sysinfo.go`:
- Line 4: The code creates a detached background context (context.Background())
for snapshot operations so Cobra command cancellation/deadlines aren't
propagated; replace the background context with the command's context by using
cmd.Context() wherever the ctx for snapshot/snapshotter operations is created
(the ctx variable assigned from context.Background() in gather-sysinfo.go) so
downstream calls (snapshot creation/collection) receive cmd.Context() instead of
a detached context.
---
Outside diff comments:
In `@pkg/performanceprofile/profilecreator/cmd/root.go`:
- Around line 158-168: The partial allocation of nodesHandlers can leak temp
dirs if makeNodesHandlers returns a non-nil slice and an error; update the error
path to cleanup any handlers already created before returning: after calling
makeNodesHandlers(pcArgs.MustGatherDirPath, pcArgs.NodePoolName, nodes) check if
err != nil and if nodesHandlers != nil iterate over each handler and call
handler.Cleanup() (log warnings on cleanup errors) before returning err; keep
the existing deferred cleanup for the success path (defer loop over
nodesHandlers calling Cleanup) so successful paths remain unchanged.
---
Nitpick comments:
In `@cmd/gather-sysinfo/gather-sysinfo.go`:
- Line 75: The return statement currently formats the wrapped error with "%v"
which prevents callers from unwrapping; change the fmt.Errorf call that returns
"error getting expected clone content: ..." to use "%w" for the second verb so
the underlying error (err) can be unwrapped by callers (i.e., replace "%v" with
"%w" in the fmt.Errorf call that returns when err != nil while getting expected
clone content).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 073f0507-b1a4-4f73-a1e6-d12cddb591a7
⛔ Files ignored due to path filters (79)
go.sumis excluded by!**/*.sumvendor/github.com/jaypipes/ghw/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/SNAPSHOT.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/alias.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/host.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/config/context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/config/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/internal/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/accelerator/accelerator_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/baseboard/baseboard_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/bios/bios_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/block/block_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/chassis/chassis_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/context/context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/cpu/cpu_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/gpu/gpu_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/linuxdmi/dmi_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/linuxpath/path_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/marshal/marshal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_cache_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/memory/memory_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/net/net_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/option/option.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/pci/pci_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/product/product_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_block_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_pci_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/clonetree_usb_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/pack.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/trace.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/snapshot/unpack.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/topology/topology_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/usb/usb_stub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jaypipes/ghw/pkg/util/util.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
cmd/gather-sysinfo/gather-sysinfo.gogo.modpkg/performanceprofile/profilecreator/cmd/info.gopkg/performanceprofile/profilecreator/cmd/root.gopkg/performanceprofile/profilecreator/ghwhandler.gopkg/performanceprofile/profilecreator/profilecreator_test.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/performanceprofile/profilecreator/cmd/info.go
- pkg/performanceprofile/profilecreator/ghwhandler.go
| node = newTestNode(worker1) | ||
| }) | ||
|
|
||
| AfterEach(func() { |
There was a problem hiding this comment.
IIRC this can be racy if we run the tests in parallel, which (IIRC again) we don't.
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
/lgtm
we can merge once coderabbit is happy
|
/retest |
1 similar comment
|
/retest |
|
e2e-hypershift-pao has some infrastructure issues, also see |
|
/retest |
1 similar comment
|
/retest |
|
/test all |
|
PR-Agent: could not fine a component named |
|
/test e2e-gcp-pao-updating-profile |
|
PR-Agent: could not fine a component named |
openshift-ci-robot
added
the
jira/valid-reference
|
@jmencak: This pull request explicitly references no jira issue. 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. |
|
/verified by ci/prow/e2e-gcp-pao ci/prow/e2e-hypershift already passed before /override ci/prow/e2e-hypershift ci/prow/e2e-hypershift-pao is an infra issue which has nothing to do with the current PR. Let's get this merged prior the branching. /override ci/prow/e2e-hypershift-pao |
openshift-ci-robot
added
the
verified
|
@jmencak: This PR has been marked as verified by 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. |
|
@jmencak: Overrode contexts on behalf of jmencak: ci/prow/e2e-hypershift, ci/prow/e2e-hypershift-pao 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. |
|
PR-Agent: could not fine a component named |
|
@jmencak: Overrode contexts on behalf of jmencak: ci/prow/e2e-hypershift, ci/prow/e2e-hypershift-pao 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. |
|
@jmencak: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Upgrade ghw library and address API breaking changes. The new ghw API no
longer provides the
WithSnapshot()function andSnapshotOptionsstruct that were available in versions prior to v0.21.3.
Changes:
ghwhandler.goto use newsnapshot.Unpack()for archive handlingWithSnapshot()withoption.WithChroot()for snapshot pathsCleanup()method toGHWHandlerfor temporary directory cleanup.tgzand.tar.gzarchive formats explicitly