MTV-5558 | Rewrite guest customization as stack-based plugin architecture#6596
Draft
yaacov wants to merge 1 commit into
Draft
MTV-5558 | Rewrite guest customization as stack-based plugin architecture#6596yaacov wants to merge 1 commit into
yaacov wants to merge 1 commit into
Conversation
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
4edf681 to
16b1b03
Compare
|
Warning Review limit reached
More reviews will be available in 25 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (42)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors virt-v2v customization into a probe-driven, plugin-based package. Adds CLI/env skip flags and entrypoint gating, updates conversion wiring, implements guestfish probing, parsers, commit backends (guestfish/virt-customize), many plugins and tests, and removes legacy customize scripts/tests and a CI job. ChangesGuest Customization System Refactoring
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6596 +/- ##
==========================================
- Coverage 15.45% 11.21% -4.25%
==========================================
Files 112 541 +429
Lines 23377 62471 +39094
==========================================
+ Hits 3613 7003 +3390
- Misses 19479 54850 +35371
- Partials 285 618 +333
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
pkg/virt-v2v/customization/probe/extract.go (1)
63-87: ⚡ Quick winRefactor
parseExtractioninto table-driven dispatch to reduce complexity.Line 63 currently chains multiple section-specific branches; this is already tripping the complexity gate and makes extension error-prone. A small dispatcher table (
start,end,handler) keeps behavior the same with less branching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/extract.go` around lines 63 - 87, Refactor parseExtraction to use a table-driven dispatcher: create a slice of descriptors each with start, end and a handler function (or closure) that accepts the extracted section and mutates the guest (instead of chaining many if blocks); for entries corresponding to parser.Ifcfg, parser.NM, parser.Interfaces call the handler to append to guest.Interfaces, for parser.Netplan have the handler append result.Interfaces and set guest.NetplanRenderer when non-empty, and for the wicked pair use two table entries or a special handler that reads both "===WICKED_START==="/"===WICKED_XML===" and "===WICKED_XML==="/"===WICKED_END===" as needed; replace the current direct calls to extractSection(...) and parser.* in parseExtraction with a loop over the dispatcher that calls extractSection and invokes the handler when the section is non-empty.pkg/virt-v2v/customization/probe/parser/ifcfg_test.go (1)
5-95: ⚡ Quick winAdd a regression test for concatenated
ifcfg-*content without blank lines.Current tests don’t cover the real extraction shape where records may be adjacent; please add a case with two
DEVICE=stanzas back-to-back to prevent parser record-merging regressions.As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/ifcfg_test.go` around lines 5 - 95, Add a new unit test (e.g., TestIfcfg_ConcatenatedRecords) that calls Ifcfg with two DEVICE stanzas concatenated directly (no blank line between them) to ensure records are not merged; construct input like "DEVICE=eth0\nIPADDR=192.168.1.10\nHWADDR=00:11:22:33:44:55\nDEVICE=eth1\nIPADDR=10.0.0.5\nHWADDR=aa:bb:cc:dd:ee:ff\n", assert Ifcfg returns two interfaces, check interfaces[0].Name=="eth0" and interfaces[1].Name=="eth1" and verify at least one IP/MAC per interface to replicate the other tests' assertions.pkg/virt-v2v/customization/probe/parser/nm_test.go (1)
62-99: ⚡ Quick winAdd a regression test for non-IP sections
Please add a case where non-
[ipv4]/[ipv6]sections containmethod=autooraddress*and assert they do not set DHCP/IPs.🧪 Suggested test case
+func TestNM_IgnoresAddressAndMethodOutsideIPSections(t *testing.T) { + section := "[connection]\ninterface-name=ens192\n\n[proxy]\nmethod=auto\naddress1=203.0.113.10/24\n" + interfaces := NM(section) + if len(interfaces) != 1 { + t.Fatalf("expected 1 interface, got %d", len(interfaces)) + } + if interfaces[0].DHCP { + t.Error("expected DHCP to remain false when method=auto is outside ipv4/ipv6") + } + if len(interfaces[0].IPv4) != 0 || len(interfaces[0].IPv6) != 0 { + t.Errorf("expected no IPs from non-ip sections, got IPv4=%v IPv6=%v", interfaces[0].IPv4, interfaces[0].IPv6) + } +}As per coding guidelines:
**/*_test.go: Coverage: Strive for high test coverage on critical logic paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/nm_test.go` around lines 62 - 99, Add a new unit test (e.g., TestNM_NonIPSectionsIgnoreAddresses) that constructs an input where non-[ipv4]/[ipv6] sections (like [proxy] or [connection] extras) include lines like "method=auto" and "address1=1.2.3.4/24,1.2.3.1" and then calls NM(section); assert that NM does not treat those lines as IP config (interfaces[*].IPv4 and IPv6 should remain empty for those sections) while a proper [ipv4]/[ipv6] section still parses correctly; use the existing helpers (NM and containsStr) and assert interface count/name as needed to ensure only true IP sections produce addresses.pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.go (1)
92-169: ⚡ Quick winAdd a regression test for malformed extension names.
Please add a case asserting files like
001_linux_run_scriptXsh/001_win_firstboot_scriptXps1are ignored. It will lock the filename contract and prevent accidental execution on loose matches.As per coding guidelines: Coverage: “Strive for high test coverage on critical logic paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.go` around lines 92 - 169, Add a regression test in plugin_test.go that creates files with malformed extensions (e.g. "001_linux_run_scriptXsh" and "001_win_firstboot_scriptXps1") in a TempDir, construct the same api.Context used in existing tests (use p.Apply, api.Context, api.GuestInfo and utils.FileSystemImpl), call p.Apply(ctx) and assert no actions are produced (actions.Execs and actions.Files are empty) for both Linux and Windows guest families; place the test alongside the existing It blocks so it exercises the same filename filtering logic in the Apply method.pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.go (1)
61-61: ⚡ Quick winTighten file path verification to exact match.
Substring matching is permissive; assert the full expected path.
As per coding guidelines "Assertions must be specific and provide informative messages on failure."Proposed test tightening
import ( + "path/filepath" "testing" @@ - Expect(actions.Files[0].GuestPath).To(ContainSubstring("990_signal_conversion_done.ps1")) + Expect(actions.Files[0].GuestPath).To(Equal(filepath.Join(api.WinFirstbootScriptsPath, "990_signal_conversion_done.ps1")))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.go` at line 61, Replace the permissive substring assertion on actions.Files[0].GuestPath with an exact-match assertion: change the Expect(...).To(ContainSubstring("990_signal_conversion_done.ps1")) to an equality check Expect(actions.Files[0].GuestPath).To(Equal("<full_expected_path_to_990_signal_conversion_done.ps1">)) using the actual full path your code should produce (e.g., the canonical guest path for the script) so the test verifies the exact file path.pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.go (1)
61-61: ⚡ Quick winAssert exact destination path for the generated file action.
Use an exact equality assertion to catch wrong-directory regressions.
As per coding guidelines "Assertions must be specific and provide informative messages on failure."Proposed test tightening
import ( + "path/filepath" "testing" @@ - Expect(actions.Files[0].GuestPath).To(ContainSubstring("200_restore_config_legacy.ps1")) + Expect(actions.Files[0].GuestPath).To(Equal(filepath.Join(api.WinFirstbootScriptsPath, "200_restore_config_legacy.ps1")))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.go` at line 61, The test currently uses a substring match on actions.Files[0].GuestPath which can miss wrong-directory regressions; replace the ContainSubstring assertion with an exact equality check: compare actions.Files[0].GuestPath to the full expected destination path (preferably using a single expected variable or filepath.Join to construct it) so the test asserts equality (Expect(...).To(Equal(expectedPath))). Ensure you reference the same actions.Files[0].GuestPath expression and update the test to fail with the exact path value on mismatch.pkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.go (1)
53-54: ⚡ Quick winUse exact guest-path assertions instead of substring checks.
ContainSubstringcan pass even if the base directory is wrong. Use exact expected paths to make failures precise.As per coding guidelines "Assertions must be specific and provide informative messages on failure."Proposed test tightening
import ( + "path/filepath" "testing" @@ - Expect(actions.Files[0].GuestPath).To(ContainSubstring("900_firstboot_init.bat")) + Expect(actions.Files[0].GuestPath).To(Equal(filepath.Join(api.WinFirstbootScriptsPath, "900_firstboot_init.bat"))) @@ - Expect(actions.Files[0].GuestPath).To(ContainSubstring("900_firstboot_init_legacy.bat")) + Expect(actions.Files[0].GuestPath).To(Equal(filepath.Join(api.WinFirstbootScriptsPath, "900_firstboot_init_legacy.bat")))Also applies to: 67-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.go` around lines 53 - 54, Replace the loose ContainSubstring assertions on actions.Files[0].GuestPath and the similar assertion at the other occurrence (lines referenced) with exact-equality checks: compute or use the expected full guest-path string (e.g. expectedGuestPath0 and expectedGuestPath1) and assert Expect(actions.Files[0].GuestPath).To(Equal(expectedGuestPath0)) and Expect(actions.Files[1].GuestPath).To(Equal(expectedGuestPath1)); update both assertions so failures show the precise mismatched path rather than a substring match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/virt-v2v/customization/commit/files.go`:
- Around line 70-85: Paths passed into guestfish commands are unquoted and will
break on spaces; update the command construction in the actions loop in
commit/files.go so all guestfish path arguments are quoted with strconv.Quote.
Specifically: change the mkdir-p invocation that writes "mkdir-p %s\n" to quote
d; change the upload invocation "upload %s %s\n" to quote both action.LocalPath
and action.GuestPath; change the write invocation "write %s %s\n" to quote
action.GuestPath (keep using strconv.Quote for action.Content as before); and
change the chmod invocation "chmod %s %s\n" to quote action.GuestPath. Use
strconv.Quote consistently so guestfish receives properly quoted paths.
In `@pkg/virt-v2v/customization/commit/scripts.go`:
- Around line 27-34: The loop over actions silently ignores unknown
api.ExecAction.Type values; add a default branch that fails fast by returning an
error (or wrapping one) when action.Type is not api.ActionFirstboot or
api.ActionRun so unsupported types are not dropped. Locate the for _, action :=
range actions loop and inside the switch on action.Type add a default case that
constructs a contextual error mentioning the offending action.Type (and
optionally its index or action.Value) and return it (update the enclosing
function signature to return error if needed) instead of silently continuing;
keep using cmd.AddArg for the two supported cases.
In `@pkg/virt-v2v/customization/customize.go`:
- Around line 31-43: In Run, validate required inputs before any dereference:
check that opts.Config != nil (and that fields you use like opts.Config.RootDisk
exist if needed) and that opts.Disks is non-nil/non-empty, returning a
descriptive error if missing; perform these checks before calling applyDefaults,
luksKeys, or probe.Guest so functions like luksKeys(opts.FileSystem,
opts.Config) and probe.Guest(opts.CommandBuilder, opts.Disks, ...) cannot panic.
Ensure error messages reference Run and the missing field (e.g., "Run: missing
Config" or "Run: no Disks provided") so callers get clear API validation
feedback.
In `@pkg/virt-v2v/customization/detect_test.go`:
- Around line 231-237: The test currently compares ordering using registryIdx,
vmwareIdx, firstbootIdx, and conversionIdx but doesn't assert each plugin is
present (indexOf returns -1 can make comparisons vacuously true); update the
test to first assert presence for each plugin by checking registryIdx,
vmwareIdx, firstbootIdx, and conversionIdx are not -1 (e.g., use
Expect(...).ToNot(Equal(-1))) before performing the existing BeNumerically
ordering assertions that reference indexOf(names, "...").
In `@pkg/virt-v2v/customization/plugins/network/registry/plugin.go`:
- Around line 118-205: The function injectComplementaryIPTemplate is doing
parsing, transformation and rendering in one large block; split it into small
helpers: (1) parseStaticIPEntries(staticIPs string) ([]parsedEntry, error) —
extract the logic that splits segments, validates parts and returns parsed
structs (use ipEntry for IP fields); (2) buildIPConfigs(parsed []parsedEntry)
[]ipConfig — collect and sort MACs, build the []ipConfig with only complementary
IPs (reuse MAC normalization with strings.ReplaceAll); and (3)
renderComplementaryTemplate(configs []ipConfig, tmplContent []byte) ([]byte,
error) — create the template.FuncMap (lower, add, len, formatIPs, formatDNS) and
perform template parsing/execution. Keep readScript call and error handling in
injectComplementaryIPTemplate and call these helpers in sequence
(injectComplementaryIPTemplate should only orchestrate: read template,
parseStaticIPEntries, buildIPConfigs, renderComplementaryTemplate) so each piece
is testable and complexity is reduced.
In
`@pkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1`:
- Around line 258-261: Set-ItemProperty that writes DefaultGateway can fail
silently; modify the write to either use -ErrorAction Stop and wrap it in a
try/catch or check the result ($? or return value) before logging success.
Specifically update the Set-ItemProperty call that writes $merged to the
"DefaultGateway" at $regPath (and the surrounding lines referencing $merged and
$interfaceAlias) so failures are caught and a failure message is logged in the
catch branch instead of always emitting the "[OK] Gateway written to registry
for $interfaceAlias" line.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go`:
- Around line 33-39: The Applicable method currently treats any non-NotExist
error as "applicable" which is wrong; update Plugin.Applicable so it only
returns true when ctx.FileSystem.Stat(ctx.Config.DynamicScriptsDir) returns nil
(i.e., the directory exists and Stat succeeded). Specifically, in
Plugin.Applicable (and referencing ctx.Config.DynamicScriptsDir and
ctx.FileSystem.Stat), replace the final return that uses os.IsNotExist(err) with
a check that err == nil (and treat any error as not applicable), and ensure
errors are not silently ignored (wrap or log if you have a common logger
convention) rather than treating permission/IO errors as success.
- Around line 24-26: windowsDynamicRegex and linuxDynamicRegex currently allow
any character before the extension because the dots in (([\w\-]*).ps1) and
(([\w\-]*).sh) are unescaped; update the regexes (used by getScriptsWithRegex
which filters by file.Name()) to escape the literal dot before the extension
(e.g., use \.ps1 and \.sh) so only files actually ending with .ps1 or .sh are
matched.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg.go`:
- Around line 22-45: The parser only flushes interfaces on blank lines causing
concatenated ifcfg files to merge records; update the DEVICE handling in
ifcfg.go so when encountering the "DEVICE" key you first append the existing
current (api.InterfaceInfo current) to interfaces if current.Name != "" ||
current.HasIPs(), then reset current.Source = "ifcfg" before setting
current.Name = val; apply the same flush-before-overwrite logic for any other
DEVICE handling site (the other DEVICE case around the later block) to prevent
cross-file contamination.
In `@pkg/virt-v2v/customization/probe/parser/nm.go`:
- Around line 40-60: The DHCP and IP parsing currently run regardless of stanza;
limit them to only run when currentSection is "ipv4" or "ipv6". In the switch
that handles keys (the block containing case "method" and case
"mac-address"/"interface-name"), move or guard the case "method" logic so it
only sets current.DHCP when currentSection == "ipv4" or currentSection == "ipv6"
as intended, and wrap the address parsing block (the if strings.HasPrefix(key,
"address") ... ) so it only executes when currentSection is "ipv4" or "ipv6" and
then append to current.IPv4 or current.IPv6 accordingly using the existing
current.IPv4/current.IPv6 variables; reference identifiers: currentSection,
current.DHCP, case "method", the address parsing block, current.IPv4, and
current.IPv6.
In `@pkg/virt-v2v/customization/probe/probe.go`:
- Around line 33-36: The return from runGuestfishScript in the extraction step
loses phase context; update the error handling around extractOutput, err :=
runGuestfishScript(cmdBuilder, disks, keys, rootDisk, extractScript) to wrap the
returned err with the extraction phase context (e.g., include "phase=extract" or
similar) before returning guest, so logs and callers get consistent actionable
context like the detection path does; modify the error return to wrap err using
your project's error-wrapping convention around the extract step in probe.go.
---
Nitpick comments:
In `@pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.go`:
- Line 61: Replace the permissive substring assertion on
actions.Files[0].GuestPath with an exact-match assertion: change the
Expect(...).To(ContainSubstring("990_signal_conversion_done.ps1")) to an
equality check
Expect(actions.Files[0].GuestPath).To(Equal("<full_expected_path_to_990_signal_conversion_done.ps1">))
using the actual full path your code should produce (e.g., the canonical guest
path for the script) so the test verifies the exact file path.
In `@pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.go`:
- Line 61: The test currently uses a substring match on
actions.Files[0].GuestPath which can miss wrong-directory regressions; replace
the ContainSubstring assertion with an exact equality check: compare
actions.Files[0].GuestPath to the full expected destination path (preferably
using a single expected variable or filepath.Join to construct it) so the test
asserts equality (Expect(...).To(Equal(expectedPath))). Ensure you reference the
same actions.Files[0].GuestPath expression and update the test to fail with the
exact path value on mismatch.
In `@pkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.go`:
- Around line 53-54: Replace the loose ContainSubstring assertions on
actions.Files[0].GuestPath and the similar assertion at the other occurrence
(lines referenced) with exact-equality checks: compute or use the expected full
guest-path string (e.g. expectedGuestPath0 and expectedGuestPath1) and assert
Expect(actions.Files[0].GuestPath).To(Equal(expectedGuestPath0)) and
Expect(actions.Files[1].GuestPath).To(Equal(expectedGuestPath1)); update both
assertions so failures show the precise mismatched path rather than a substring
match.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.go`:
- Around line 92-169: Add a regression test in plugin_test.go that creates files
with malformed extensions (e.g. "001_linux_run_scriptXsh" and
"001_win_firstboot_scriptXps1") in a TempDir, construct the same api.Context
used in existing tests (use p.Apply, api.Context, api.GuestInfo and
utils.FileSystemImpl), call p.Apply(ctx) and assert no actions are produced
(actions.Execs and actions.Files are empty) for both Linux and Windows guest
families; place the test alongside the existing It blocks so it exercises the
same filename filtering logic in the Apply method.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 63-87: Refactor parseExtraction to use a table-driven dispatcher:
create a slice of descriptors each with start, end and a handler function (or
closure) that accepts the extracted section and mutates the guest (instead of
chaining many if blocks); for entries corresponding to parser.Ifcfg, parser.NM,
parser.Interfaces call the handler to append to guest.Interfaces, for
parser.Netplan have the handler append result.Interfaces and set
guest.NetplanRenderer when non-empty, and for the wicked pair use two table
entries or a special handler that reads both
"===WICKED_START==="/"===WICKED_XML===" and
"===WICKED_XML==="/"===WICKED_END===" as needed; replace the current direct
calls to extractSection(...) and parser.* in parseExtraction with a loop over
the dispatcher that calls extractSection and invokes the handler when the
section is non-empty.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg_test.go`:
- Around line 5-95: Add a new unit test (e.g., TestIfcfg_ConcatenatedRecords)
that calls Ifcfg with two DEVICE stanzas concatenated directly (no blank line
between them) to ensure records are not merged; construct input like
"DEVICE=eth0\nIPADDR=192.168.1.10\nHWADDR=00:11:22:33:44:55\nDEVICE=eth1\nIPADDR=10.0.0.5\nHWADDR=aa:bb:cc:dd:ee:ff\n",
assert Ifcfg returns two interfaces, check interfaces[0].Name=="eth0" and
interfaces[1].Name=="eth1" and verify at least one IP/MAC per interface to
replicate the other tests' assertions.
In `@pkg/virt-v2v/customization/probe/parser/nm_test.go`:
- Around line 62-99: Add a new unit test (e.g.,
TestNM_NonIPSectionsIgnoreAddresses) that constructs an input where
non-[ipv4]/[ipv6] sections (like [proxy] or [connection] extras) include lines
like "method=auto" and "address1=1.2.3.4/24,1.2.3.1" and then calls NM(section);
assert that NM does not treat those lines as IP config (interfaces[*].IPv4 and
IPv6 should remain empty for those sections) while a proper [ipv4]/[ipv6]
section still parses correctly; use the existing helpers (NM and containsStr)
and assert interface count/name as needed to ensure only true IP sections
produce addresses.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8dc6185e-7814-4df7-8f64-abf96367bdc7
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (132)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/testdata/valid_config.yml
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/customize.go
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- .github/workflows/pull-request.yml
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
4 times, most recently
from
ba7e8bd to
0b22259
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go (1)
7-107: ⚡ Quick winPrefer table-driven tests for these related cases.
The cases are good, but they’re repetitive. Consolidating them into table-driven tests will improve readability and make future additions less error-prone.
As per coding guidelines, for tests: "Readability: Use table-driven tests for multiple cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go` around lines 7 - 107, Consolidate the repetitive test cases into table-driven tests: replace the separate TestParseEntries_SingleMAC, TestParseEntries_MultipleIPs, and TestParseEntries_SkipsMalformed with a single TestParseEntries table that iterates cases (name, input string, expected MAC keys and expected IPs/gateway/prefix/DNS) and uses t.Run for each case while calling ParseEntries and asserting results; likewise replace the multiple TestBuildComplementaryConfigs_* tests with a single TestBuildComplementaryConfigs table that contains entries for "DropsFirstIP", "SingleIPSkipped", and "SortedByMAC" cases (provide macMap input and expected configs length, MAC order and IP values) and in each t.Run call BuildComplementaryConfigs and validate the returned slice; reference ParseEntries, BuildComplementaryConfigs, IPEntry, and the config struct fields (MAC, IPs, IPEntry.IP) when implementing assertions.pkg/virt-v2v/customization/resolve.go (1)
17-48: ⚡ Quick winAdd direct unit tests for plugin ordering and filtering contract.
ResolveandAllPluginsdefine critical execution order and applicability behavior, but this file currently has no direct tests in the provided changes. Please add focused tests that lock the plugin order and verify filtering with representative contexts.As per coding guidelines, "coverage: Make sure that the code has unit tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/resolve.go` around lines 17 - 48, Add unit tests that lock the execution order returned by AllPlugins() and verify filtering behavior of Resolve(ctx). Write tests that call AllPlugins() and assert the slice order matches the expected sequence of plugin types (udev.Plugin, registry.Plugin, netsh.Plugin, vmware.Plugin, dynamic.Plugin, firstboot.Plugin, diskrestore.Plugin, conversiondone.Plugin), and separate tests that construct representative api.Context values and assert Resolve(ctx) returns only the plugins whose Applicable(ctx) would be true (mock or create minimal contexts exercising Linux vs Windows and legacy/modern paths). Use the api.Plugin interface and the Applicable method to drive filtering checks and ensure tests fail if order or applicability changes.pkg/virt-v2v/config/variables.go (1)
179-180: ⚡ Quick winAdd unit tests for
SkipConversion/SkipCustomizebehavior.These flags change config control flow, especially the early
validate()return path. Please add focused tests that lock expected combinations and guard against regressions.As per coding guidelines: coverage requires unit tests for changed logic paths.
Also applies to: 287-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/config/variables.go` around lines 179 - 180, Add unit tests that exercise the config flag combinations for SkipConversion and SkipCustomize to lock expected control flow and validate early-return behavior in validate(). Specifically, create tests that instantiate the config (using the same constructor or New/Load function used in variables.go), set SkipConversion and SkipCustomize via flag parsing or by calling getEnvBool stubs/mocks, and assert validate() returns the expected result/path (including the early return when SkipConversion is true and interactions with SkipCustomize). Cover all relevant combinations (both false, SkipConversion true, SkipCustomize true, both true) and mirror the same cases referenced around the other occurrences (e.g., the lines near 287-289) to prevent regressions.pkg/virt-v2v/customization/plugins/network/netsh/plugin_test.go (1)
24-68: ⚡ Quick winRefactor
Applicablescenarios into a table-driven spec.These
Applicabletests are mostly the same shape with different inputs. Converting them to aDescribeTablewill reduce duplication and make future cases easier to add.As per coding guidelines: Readability in tests should use table-driven tests for multiple cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/netsh/plugin_test.go` around lines 24 - 68, The tests for the Applicable method are repetitive; refactor the Describe("Applicable", ...) block into a Ginkgo DescribeTable that exercises p.Applicable with entries for each scenario. Create a table that defines inputs (an api.Context containing Guest: &api.GuestInfo{OS: api.GuestOS{Family: ...}} and Config: &config.AppConfig{...}) and the expected bool, then replace the individual It blocks with Entry(...) rows calling Expect(p.Applicable(ctx)).To(Equal(expected)). Keep references to the Applicable method and the used types (api.Context, api.GuestInfo, api.GuestOS, config.AppConfig) so each case (Windows/Linux, empty StaticIPs, registry flag set, no legacy drivers) is represented as an Entry.pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go (1)
53-53: ⚡ Quick winUse structured logging (pkg/lib/logging) instead of
fmt.Printfin virt-v2v customization paths.
pkg/virt-v2v/customizationcurrently prints directly to stdout, while the repo provides a structured Zap/logr logger (pkg/lib/logging) for context-rich logs. Emit key/value context (e.g., plugin name, script path) via that logger—especially in:
pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go(lines 53, 70)pkg/virt-v2v/customization/customize.go(line 66)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go` at line 53, Replace the plain stdout print in the dynamic plugin with the structured logger from pkg/lib/logging: stop using fmt.Printf("Adding windows dynamic script '%s'\n", script.path) and instead obtain a logger (from pkg/lib/logging) and emit a structured message with key/value pairs including the plugin name and script.path; do the same for the other occurrences called out (the second print in plugin.go and the log in customize.go). Locate the call that prints script.path (reference symbol: script.path and the string "Adding windows dynamic script") and the related plugin logging sites, create or get a logger instance (use the repo's pkg/lib/logging entrypoint), and replace printf calls with logger.Info or logger.WithValues(...).Info(...) including keys like "plugin" and "scriptPath".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/virt-v2v/customization/customize.go`:
- Around line 31-127: Add table-driven unit tests covering Run and luksKeys: for
Run create cases that validate missing Config and empty Disks return the
expected wrapped errors, plugin Apply errors are wrapped (use a fake plugin
returned by Resolve or mock Resolve to return a plugin whose Apply returns an
error), that file actions trigger a call path to commit.Files (mock/stub
commit.Files to record invocation) and that exec actions trigger commit.Scripts
(stub commit.Scripts to verify it's called only when execs present), and that
probing failures from probe.Guest are propagated; for luksKeys add cases for
NbdeClevis true, empty Luksdir, os.IsNotExist from fs.Stat, Stat returning other
errors, GetFilesInPath returning error, and successful key-list generation
(mock/fs implementations for utils.FileSystem and utils.GetFilesInPath),
ensuring each case asserts the returned slice and wrapped error messages from
luksKeys and Run.
In
`@pkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.bat`:
- Around line 2-8: Change the unquoted environment assignments to the safe
quoted SET form so values containing spaces or special characters are preserved:
replace lines that assign firstboot, scripts and scripts_done (symbols:
firstboot, scripts, scripts_done) with set "firstboot=C:\Program
Files\Guestfs\Firstboot" and set "scripts=%firstboot%\scripts" and set
"scripts_done=%firstboot%\scripts-done" (keep setlocal enabledelayedexpansion
as-is) and ensure subsequent usage (if not exist "%scripts_done%" mkdir
"%scripts_done%") continues to reference the variables with quotes.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render.go`:
- Around line 20-27: The template parse and execute error returns lack
contextual messages; update the error returns around
template.New("complementaryIPs").Funcs(funcMap).Parse(string(tmplContent)) and
tmpl.Execute(&buf, configs) to wrap the original err with descriptive context
(e.g., "parsing template complementaryIPs" and "executing template
complementaryIPs") using fmt.Errorf("%s: %w", ...) or the project's standard
error-wrapping helper so the returned error includes the operation and the
underlying err and still propagates.
- Around line 47-79: The functions formatIPs and formatDNS currently inject raw
IP/DNS values into single-quoted PowerShell literals and must escape single
quotes to avoid breaking the script or enabling injection; update formatIPs
(loop over ips and when writing ip.IP) and formatDNS (loop over dns and when
writing d) to replace any single-quote characters with two single-quotes
(PowerShell escaping) before writing, or add a small helper
escapePowerShellLiteral(s string) and call it from both functions so all
inserted values are sanitized.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.go`:
- Around line 30-39: The ParseEntries function currently drops malformed static
IP segments silently; change ParseEntries to return a diagnostic collection
(e.g., ([]error) or ([]error, []warning) or a structured []Diagnostic) alongside
the successful parsed results, collect and append contextual wrapped errors
whenever a segment fails parsing (for example when parts length != 2 or ipParts
length < 5), and update all callers to receive and handle/propagate these
diagnostics instead of ignoring them; ensure you reference and update the
ParseEntries signature and its callers, wrap errors with context like
"ParseEntries: malformed segment '<segment>'" and do not swallow any parse
failures.
In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go`:
- Around line 26-37: The loop in Plugin.Apply uses
parseStaticIPs(ctx.Config.StaticIPs) values directly when building udev rules;
validate each pair before appending by verifying pair.MAC with net.ParseMAC and
pair.IP with net.ParseIP (or equivalent validators) and ensure ifaceName from
ctx.Guest.InterfaceForIP(pair.IP) is non-empty; on validation failure either
return an error from Apply (fail fast) or skip the entry and log a clear error,
so malformed MAC/IP tokens cannot produce invalid udev rule lines.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go`:
- Around line 14-18: ReadDir results are filesystem-dependent so you must sort
directory entries before building actions; after each call to ReadDir in
pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go (the occurrences
around the imports and the block covering lines ~93-113), collect entry names
(or use the DirEntry slice) and sort them (e.g., sort.Slice(entries, func(i,j
int) bool { return entries[i].Name() < entries[j].Name() }) or
sort.Strings(names)) so scripts named with numeric prefixes (NNN_...) execute
deterministically; apply this sorting right after the ReadDir call and before
any iteration that generates actions.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 11-61: buildExtractionScript is too complex; split each section
into small helper builders. Create helpers like buildIfcfgCommands(guest
*api.GuestInfo) []string, buildNMCommands(guest *api.GuestInfo) []string,
buildNetplanCommands(guest *api.GuestInfo) []string,
buildInterfacesCommands(guest *api.GuestInfo) []string and
buildWickedCommands(guest *api.GuestInfo) []string that contain the per-section
command assembly (including their echo markers and glob/prefix strings), keep
the OS check (cat /etc/os-release) in buildExtractionScript, call each helper
conditionally from buildExtractionScript to append their returned slices to
cmds, and finally join and return the script exactly as before; ensure function
signatures, imports and behavior remain unchanged and adjust unit tests if they
reference the previous inlined behavior.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg.go`:
- Line 13: The top-of-file parser contract comment in ifcfg.go is stale: it
states concatenated input is separated only by blank lines but the parser also
splits on the "DEVICE" key; update that comment to reflect actual behavior
(mention that entries are split on blank lines or on a new DEVICE= line),
reference the parser logic handling DEVICE and any functions/types like the
ifcfg parser or split routine in this file (e.g., the code that looks for the
"DEVICE" key), and ensure the comment clearly documents both splitting rules so
future maintainers won't regress the parser.
In `@pkg/virt-v2v/customization/probe/parser/nm.go`:
- Around line 19-66: The NM parser currently swallows bufio.Scanner errors;
change the NM function signature from NM(section string) []api.InterfaceInfo to
NM(section string) ([]api.InterfaceInfo, error), check scanner.Err() after the
Scan loop and return the error (or nil, interfaces) if non-nil, and update all
callers (e.g., parseExtraction) to propagate the error; apply the same
signature/error-check pattern to the other parsers (functions in
parser/interfaces.go and parser/ifcfg.go) so they also validate scanner.Err()
and return errors instead of silently truncating interface data.
In `@pkg/virt-v2v/customization/probe/parser/wicked.go`:
- Around line 73-83: parseLeaseXML currently swallows all errors from
decodeOneLease and returns partial results; change its signature to return
([]string, error), call decodeOneLease in a loop and treat io.EOF as normal
termination but wrap and return any other error (using fmt.Errorf/ %w) so
callers can handle failures, then update all call sites to handle the returned
error; apply the same error-propagation change to the other similar parsing
block referenced (around the second occurrence) so DecodeElement/decoder errors
are not ignored.
---
Nitpick comments:
In `@pkg/virt-v2v/config/variables.go`:
- Around line 179-180: Add unit tests that exercise the config flag combinations
for SkipConversion and SkipCustomize to lock expected control flow and validate
early-return behavior in validate(). Specifically, create tests that instantiate
the config (using the same constructor or New/Load function used in
variables.go), set SkipConversion and SkipCustomize via flag parsing or by
calling getEnvBool stubs/mocks, and assert validate() returns the expected
result/path (including the early return when SkipConversion is true and
interactions with SkipCustomize). Cover all relevant combinations (both false,
SkipConversion true, SkipCustomize true, both true) and mirror the same cases
referenced around the other occurrences (e.g., the lines near 287-289) to
prevent regressions.
In `@pkg/virt-v2v/customization/plugins/network/netsh/plugin_test.go`:
- Around line 24-68: The tests for the Applicable method are repetitive;
refactor the Describe("Applicable", ...) block into a Ginkgo DescribeTable that
exercises p.Applicable with entries for each scenario. Create a table that
defines inputs (an api.Context containing Guest: &api.GuestInfo{OS:
api.GuestOS{Family: ...}} and Config: &config.AppConfig{...}) and the expected
bool, then replace the individual It blocks with Entry(...) rows calling
Expect(p.Applicable(ctx)).To(Equal(expected)). Keep references to the Applicable
method and the used types (api.Context, api.GuestInfo, api.GuestOS,
config.AppConfig) so each case (Windows/Linux, empty StaticIPs, registry flag
set, no legacy drivers) is represented as an Entry.
In
`@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go`:
- Around line 7-107: Consolidate the repetitive test cases into table-driven
tests: replace the separate TestParseEntries_SingleMAC,
TestParseEntries_MultipleIPs, and TestParseEntries_SkipsMalformed with a single
TestParseEntries table that iterates cases (name, input string, expected MAC
keys and expected IPs/gateway/prefix/DNS) and uses t.Run for each case while
calling ParseEntries and asserting results; likewise replace the multiple
TestBuildComplementaryConfigs_* tests with a single
TestBuildComplementaryConfigs table that contains entries for "DropsFirstIP",
"SingleIPSkipped", and "SortedByMAC" cases (provide macMap input and expected
configs length, MAC order and IP values) and in each t.Run call
BuildComplementaryConfigs and validate the returned slice; reference
ParseEntries, BuildComplementaryConfigs, IPEntry, and the config struct fields
(MAC, IPs, IPEntry.IP) when implementing assertions.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go`:
- Line 53: Replace the plain stdout print in the dynamic plugin with the
structured logger from pkg/lib/logging: stop using fmt.Printf("Adding windows
dynamic script '%s'\n", script.path) and instead obtain a logger (from
pkg/lib/logging) and emit a structured message with key/value pairs including
the plugin name and script.path; do the same for the other occurrences called
out (the second print in plugin.go and the log in customize.go). Locate the call
that prints script.path (reference symbol: script.path and the string "Adding
windows dynamic script") and the related plugin logging sites, create or get a
logger instance (use the repo's pkg/lib/logging entrypoint), and replace printf
calls with logger.Info or logger.WithValues(...).Info(...) including keys like
"plugin" and "scriptPath".
In `@pkg/virt-v2v/customization/resolve.go`:
- Around line 17-48: Add unit tests that lock the execution order returned by
AllPlugins() and verify filtering behavior of Resolve(ctx). Write tests that
call AllPlugins() and assert the slice order matches the expected sequence of
plugin types (udev.Plugin, registry.Plugin, netsh.Plugin, vmware.Plugin,
dynamic.Plugin, firstboot.Plugin, diskrestore.Plugin, conversiondone.Plugin),
and separate tests that construct representative api.Context values and assert
Resolve(ctx) returns only the plugins whose Applicable(ctx) would be true (mock
or create minimal contexts exercising Linux vs Windows and legacy/modern paths).
Use the api.Plugin interface and the Applicable method to drive filtering checks
and ensure tests fail if order or applicability changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b84765fd-2192-4f6d-bfc9-2e956733a286
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (136)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/staticip/render.gopkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.gopkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- .github/workflows/pull-request.yml
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/testdata/valid_config.yml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/customize.go
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
0b22259 to
3ec6152
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go (1)
19-56: ⚡ Quick winAdd a regression test for PowerShell single-quote escaping.
Given
formatIPs/formatDNSnow sanitize literals, add a case with embedded'in IP/DNS input and assert doubled quotes in output. This protects a security-relevant path from regression.As per coding guidelines: coverage: “Make sure that the code has unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go` around lines 19 - 56, Add a regression unit test to verify PowerShell single-quote escaping: update pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go to include an IP/DNS entry containing an embedded single-quote (e.g., "10.0.0.2'evil" and "8.8.8.8'bad") and call RenderComplementaryTemplate (same as TestRenderComplementaryTemplate); assert that the output uses doubled single-quotes ('' ) for those literals (check for "'10.0.0.2''evil'" and "'8.8.8.8''bad'"), ensuring formatIPs/formatDNS sanitization is effective and prevents regression.pkg/virt-v2v/customization/customize.go (1)
85-88: ⚡ Quick winSkip script execution when there are no exec actions.
Calling
commit.Scriptsunconditionally adds an unnecessary external call path whenallExecsis empty.Proposed change
- // Phase 3: Execute scripts via virt-customize (only if needed) - if err := commit.Scripts(opts.CommandBuilder, opts.Disks, keys, allExecs); err != nil { - return fmt.Errorf("virt-customize exec: %w", err) - } + // Phase 3: Execute scripts via virt-customize (only if needed) + if len(allExecs) > 0 { + if err := commit.Scripts(opts.CommandBuilder, opts.Disks, keys, allExecs); err != nil { + return fmt.Errorf("virt-customize exec: %w", err) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/customize.go` around lines 85 - 88, The code always calls commit.Scripts(opts.CommandBuilder, opts.Disks, keys, allExecs) even when there are no exec actions, causing an unnecessary external invocation; update the caller so it skips calling commit.Scripts when allExecs is empty or nil by adding a guard that checks len(allExecs) == 0 (or allExecs == nil) and only invokes commit.Scripts when there is at least one exec, keeping the existing error handling and return semantics otherwise.pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go (1)
79-84: ⚡ Quick winAvoid hardcoding script count in the Apply test.
HaveLen(5)can become stale whendriverRemovalScriptschanges. Assert againstlen(driverRemovalScripts)to keep the test aligned with the source of truth.As per coding guidelines: **Readability** expects tests to be clear and durable.Proposed change
- Expect(actions.Files).To(HaveLen(5)) + Expect(actions.Files).To(HaveLen(len(driverRemovalScripts)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go` around lines 79 - 84, The test hardcodes the expected number of scripts (Expect(actions.Files).To(HaveLen(5))) which can break when driverRemovalScripts changes; update the assertion to compare against the actual source of truth by replacing the literal 5 with len(driverRemovalScripts) so the test checks Expect(actions.Files).To(HaveLen(len(driverRemovalScripts))) and remains in sync with driverRemovalScripts when its contents change (refer to actions.Files and driverRemovalScripts in plugin_test.go).pkg/virt-v2v/customization/probe/parser/netplan_test.go (1)
18-192: ⚡ Quick winMark independent test cases as parallel.
These cases are hermetic and can run concurrently; adding
t.Parallel()at the start of each test improves runtime and catches accidental shared-state coupling earlier.As per coding guidelines:
**/*_test.go: “Parallelism: ensure tests are independent as much as possible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/netplan_test.go` around lines 18 - 192, Add t.Parallel() as the first action in each test function (TestNetplan, TestNetplan_4SpaceIndent, TestNetplan_MACAddress, TestNetplan_MultipleDocuments, TestNetplan_Renderer, TestNetplan_IPv6, TestNetplan_EmptyEthernets, TestNetplan_EmptyInput, TestNetplan_InvalidYAML) so they run concurrently; open each function and insert a single t.Parallel() call at the top to mark the case parallel and ensure no shared mutable state remains.pkg/virt-v2v/customization/probe/parser/ifcfg.go (1)
17-76: 🏗️ Heavy liftBreak
Ifcfginto focused parsing helpers.
Ifcfgcurrently mixes scan control, record-boundary logic, and field decoding in one block. Splitting into small helpers (e.g.,flushCurrent,handleDevice,handleIPv6Secondaries,handleIPv4Key) will reduce complexity and make parser behavior easier to validate and maintain.As per coding guidelines: “Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/ifcfg.go` around lines 17 - 76, The Ifcfg function mixes scanning, record-boundary logic, and field decoding making it hard to test and maintain; refactor by extracting small helpers: create flushCurrent(current *api.InterfaceInfo, interfaces *[]api.InterfaceInfo) to handle appending/resetting the current record, handleDevice(current *api.InterfaceInfo, val string) to encapsulate DEVICE/NAME logic, handleIPv6Secondaries(current *api.InterfaceInfo, val string) to parse IPV6ADDR_SECONDARIES, and handleIPv4Key(current *api.InterfaceInfo, key, val string) to centralize the IPADDR* default case; update the main Ifcfg scanner loop to call these helpers and keep error handling (scanner.Err) unchanged so behavior remains equivalent but clearer and easier to test.pkg/virt-v2v/customization/commit/files_test.go (1)
10-157: ⚡ Quick winRun these unit tests in parallel.
Each test is independent and can call
t.Parallel()to improve execution time and guard against future shared-state coupling.As per coding guidelines:
**/*_test.go: “Parallelism: ensure tests are independent as much as possible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/commit/files_test.go` around lines 10 - 157, Add t.Parallel() at the start of every test function so each TestBuildScript_* runs in parallel (e.g., TestBuildScript_Upload, TestBuildScript_Write, TestBuildScript_Chmod, TestBuildScript_MultipleActions, TestBuildScript_Empty, TestBuildScript_WriteSpecialChars, TestBuildScript_UploadWithPermissions, TestBuildScript_EmptySlice, TestBuildScript_MkdirP); insert the call as the first statement in each test body (immediately after the function's opening brace) and ensure no tests depend on shared mutable state—if any global/shared variables are used by buildScript or the tests, protect or copy them before enabling parallelism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/virt-v2v/entrypoint.go`:
- Around line 101-105: The comment above the guard uses "remote migrations" but
the condition checks convert.IsLocalMigration; update the comment to describe
the local-migration case (e.g., explain that for local migrations the controller
cannot reach the conversion pod and therefore this pod exposes parameters via a
server and awaits termination request), so that the explanatory text matches the
convert.IsLocalMigration check and refers to the same components (virt-v2v /
virt-v2v-inspector, server exposing parameters, controller terminating pod).
In `@pkg/virt-v2v/customization/commit/files.go`:
- Around line 84-86: Validate action.Permissions before writing the chmod line:
ensure it matches an octal mode pattern (e.g. a regex like ^[0-7]{3,4}$ or
^0?[0-7]{3}$) and fail fast (return an error or skip emitting the command) if it
does not, instead of interpolating it directly; update the code around the
fmt.Fprintf call that uses action.Permissions and
strconv.Quote(action.GuestPath) to perform this check and handle invalid values
by returning an error with context or logging and omitting the chmod emission.
In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin.go`:
- Line 56: Replace host-dependent filepath.Join usage with path.Join when
constructing guest filesystem paths: in the GuestPath assignment that currently
uses filepath.Join(api.WinFirstbootScriptsPath, name) switch to
path.Join(api.WinFirstbootScriptsPath, name) so guest paths (e.g., in GuestPath
and other places using api.WinFirstbootScriptsPath) always use forward slashes;
search for other occurrences of filepath.Join with api.WinFirstbootScriptsPath
(and similar guest-path constants) and change them to path.Join to avoid
injecting host-specific separators.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.go`:
- Around line 32-36: The parser in ParseEntries iterates segments from
strings.Split(staticIPs, "_") and currently treats empty tokens (from empty
input or trailing underscores) as malformed; update the loop in ParseEntries to
skip blank segments (e.g., if segment == "" or strings.TrimSpace(segment) == "")
before attempting to SplitN and appending warnings so empty tokens do not
produce false malformed-segment warnings; reference the loop over segment, the
staticIPs input, and the warnings slice where malformed-segment errors are
appended.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.go`:
- Around line 37-43: Replace hard-coded "/nonexistent/path" with a deterministic
non-existent path created under GinkgoT().TempDir(); e.g., call tmp :=
GinkgoT().TempDir() and set ctx.Config.DynamicScriptsDir = filepath.Join(tmp,
"missing") so the directory is guaranteed not to exist; update both tests that
construct the Context (the ones that call p.Applicable(ctx)) to use this
tmp-based path and import filepath if needed.
In `@pkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.go`:
- Around line 40-45: The Apply method in Plugin currently treats any non-Windows
guest as Linux; change Plugin.Apply to inspect the guest OS family (use
ctx.Guest.OS or an OS family enum like OSFamilyUnknown) with an explicit switch:
call addWindowsDynamic for Windows, addLinuxDynamic for Linux, and for
OSFamilyUnknown (or any default/unsupported case) return either an empty
api.Actions{} and nil or return a clear error instead of enqueuing Linux
actions; ensure you reference Plugin.Apply, addWindowsDynamic, addLinuxDynamic,
ctx.Guest.OS (or OSFamilyUnknown) and api.Actions when making the change.
- Around line 54-55: Replace direct stdout prints with structured logging: add a
logger field to api.Context (or obtain the repo's global logger) and use it in
plugin.go where the code currently does fmt.Printf("Adding windows dynamic
script '%s'", script.path) and the similar linux message; call the logger with
structured fields (e.g., plugin="dynamic-script", path=script.path) and an
Info-level message when appending to actions.Files (api.FileAction). Also update
pkg/virt-v2v/customization/plugins/network/registry/plugin.go to replace the
plain Warning: %v print with the same structured logger (include plugin name and
relevant context). Ensure all call sites that construct or pass api.Context are
updated to supply the new logger so logs can be correlated.
In `@pkg/virt-v2v/customization/probe/parser/netplan.go`:
- Around line 44-46: The YAML parse error for each document is currently ignored
(yaml.Unmarshal([]byte(doc), &cfg) followed by continue); instead, detect and
propagate the error with context so callers can decide how to handle failures.
Update the parsing loop in netplan.go to check err from yaml.Unmarshal, wrap it
with context referencing the document or index (e.g., "parsing netplan
document"), and either return the error immediately from the containing function
or collect errors into an aggregate and return that aggregate at the end;
reference the yaml.Unmarshal call, the cfg variable, and the surrounding parse
function to locate where to add the error wrapping/propagation.
In `@pkg/virt-v2v/customization/probe/parser/nm.go`:
- Around line 14-70: The NM function is doing scanning, state transitions, key
dispatch and finalization all inline; refactor by extracting at least two
helpers: one parseLine(current *api.InterfaceInfo, currentSection *string, line
string) error that handles trimming, section detection, key/value parsing and
updates fields (handle "interface-name", "mac-address", "method" and address
extraction into IPv4/IPv6), and another finalizeInterface(interfaces
*[]api.InterfaceInfo, current *api.InterfaceInfo) that appends current if
current.Name != "" || current.HasIPs() and resets current to
api.InterfaceInfo{Source:"nm-connection"}; keep NM to create the scanner, loop
calling parseLine for each line and calling finalizeInterface when a new
[connection] section or at end, and preserve existing behavior and error
handling (scanner.Err) and symbols NM, InterfaceInfo.HasIPs, Source, IPv4, IPv6,
DHCP, Name, MAC.
---
Nitpick comments:
In `@pkg/virt-v2v/customization/commit/files_test.go`:
- Around line 10-157: Add t.Parallel() at the start of every test function so
each TestBuildScript_* runs in parallel (e.g., TestBuildScript_Upload,
TestBuildScript_Write, TestBuildScript_Chmod, TestBuildScript_MultipleActions,
TestBuildScript_Empty, TestBuildScript_WriteSpecialChars,
TestBuildScript_UploadWithPermissions, TestBuildScript_EmptySlice,
TestBuildScript_MkdirP); insert the call as the first statement in each test
body (immediately after the function's opening brace) and ensure no tests depend
on shared mutable state—if any global/shared variables are used by buildScript
or the tests, protect or copy them before enabling parallelism.
In `@pkg/virt-v2v/customization/customize.go`:
- Around line 85-88: The code always calls commit.Scripts(opts.CommandBuilder,
opts.Disks, keys, allExecs) even when there are no exec actions, causing an
unnecessary external invocation; update the caller so it skips calling
commit.Scripts when allExecs is empty or nil by adding a guard that checks
len(allExecs) == 0 (or allExecs == nil) and only invokes commit.Scripts when
there is at least one exec, keeping the existing error handling and return
semantics otherwise.
In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go`:
- Around line 79-84: The test hardcodes the expected number of scripts
(Expect(actions.Files).To(HaveLen(5))) which can break when driverRemovalScripts
changes; update the assertion to compare against the actual source of truth by
replacing the literal 5 with len(driverRemovalScripts) so the test checks
Expect(actions.Files).To(HaveLen(len(driverRemovalScripts))) and remains in sync
with driverRemovalScripts when its contents change (refer to actions.Files and
driverRemovalScripts in plugin_test.go).
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go`:
- Around line 19-56: Add a regression unit test to verify PowerShell
single-quote escaping: update
pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go to
include an IP/DNS entry containing an embedded single-quote (e.g.,
"10.0.0.2'evil" and "8.8.8.8'bad") and call RenderComplementaryTemplate (same as
TestRenderComplementaryTemplate); assert that the output uses doubled
single-quotes ('' ) for those literals (check for "'10.0.0.2''evil'" and
"'8.8.8.8''bad'"), ensuring formatIPs/formatDNS sanitization is effective and
prevents regression.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg.go`:
- Around line 17-76: The Ifcfg function mixes scanning, record-boundary logic,
and field decoding making it hard to test and maintain; refactor by extracting
small helpers: create flushCurrent(current *api.InterfaceInfo, interfaces
*[]api.InterfaceInfo) to handle appending/resetting the current record,
handleDevice(current *api.InterfaceInfo, val string) to encapsulate DEVICE/NAME
logic, handleIPv6Secondaries(current *api.InterfaceInfo, val string) to parse
IPV6ADDR_SECONDARIES, and handleIPv4Key(current *api.InterfaceInfo, key, val
string) to centralize the IPADDR* default case; update the main Ifcfg scanner
loop to call these helpers and keep error handling (scanner.Err) unchanged so
behavior remains equivalent but clearer and easier to test.
In `@pkg/virt-v2v/customization/probe/parser/netplan_test.go`:
- Around line 18-192: Add t.Parallel() as the first action in each test function
(TestNetplan, TestNetplan_4SpaceIndent, TestNetplan_MACAddress,
TestNetplan_MultipleDocuments, TestNetplan_Renderer, TestNetplan_IPv6,
TestNetplan_EmptyEthernets, TestNetplan_EmptyInput, TestNetplan_InvalidYAML) so
they run concurrently; open each function and insert a single t.Parallel() call
at the top to mark the case parallel and ensure no shared mutable state remains.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfda1981-2c52-4553-9dd1-1fb2bdb061eb
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (136)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/staticip/render.gopkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.gopkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- .github/workflows/pull-request.yml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/testdata/valid_config.yml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/customize.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
3ec6152 to
2bb1af5
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/virt-v2v/entrypoint.go (1)
34-113:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd unit coverage for the new entrypoint control flow.
This PR changes multiple high-impact branches (
SkipConversion, remote inspection short-circuit,SkipCustomize, local-migration server start), but the PR coverage report shows this file at 0%. Please add focused tests around these branch combinations to prevent regressions.As per coding guidelines: “coverage: Make sure that the code has unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/virt-v2v/entrypoint.go` around lines 34 - 113, Write unit tests for cmd/virt-v2v/entrypoint.go that exercise the main control-flow branches: when env.SkipConversion is true (no conversion calls), when env.IsRemoteInspection is true (mock convert.RunRemoteV2vInspection and assert it’s called and function returns early), for in-place conversion with convert.LibvirtUrl set (mock convert.GetDomainXML to return XML, ensure WriteFile path convert.LibvirtDomainFile and convert.RunVirtV2vInPlace are invoked) and without LibvirtUrl (mock RunVirtV2vInPlaceDisk), for the non-in-place path (mock RunVirtV2v), ensure RunVirtV2VInspection is invoked after conversion, when env.SkipCustomize is true vs false (mock convert.RunCustomize to return error and assert server.AddWarning is called), and when convert.IsLocalMigration is true (mock server.Server.Start to simulate success/failure). Use table-driven tests and replace/spy the functions on the convert and server types (RunRemoteV2vInspection, GetDomainXML, RunVirtV2vInPlace, RunVirtV2vInPlaceDisk, RunVirtV2v, RunVirtV2VInspection, RunCustomize, and server.Server.Start) to verify each branch and error handling.
♻️ Duplicate comments (1)
pkg/virt-v2v/customization/probe/parser/wicked.go (1)
129-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
DecodeElementfailures instead of silently skipping them.If XML decode fails here, the parser currently suppresses the error and continues with partial data. That can produce wrong interface/IP mapping without surfacing a failure.
Suggested fix
case xml.StartElement: depth++ if t.Name.Local == "address" { var content string - if err := decoder.DecodeElement(&content, &t); err == nil { - address = strings.SplitN(content, "/", 2)[0] - } + if err := decoder.DecodeElement(&content, &t); err != nil { + return address, fmt.Errorf("decoding <address>: %w", err) + } + address = strings.SplitN(content, "/", 2)[0] depth-- }As per coding guidelines:
**/*.go— “Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/wicked.go` around lines 129 - 131, The DecodeElement call currently swallows errors and continues, leading to silent failures; change the logic around decoder.DecodeElement(&content, &t) so that if err != nil you wrap it with context (mentioning the element/token and the interface being parsed) and return the error instead of ignoring it; update the surrounding function (the parser routine that declares decoder, t and address) to propagate that returned error to callers so callers can handle or report XML decode failures.
🧹 Nitpick comments (5)
pkg/virt-v2v/customization/probe/extract.go (1)
95-139: ⚡ Quick win
parseExtractioncan be simplified with a shared section-parse helper.The repeated “extract → parse → append” blocks keep this function above the complexity threshold. Extracting a helper for common parser flow should reduce complexity without behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/extract.go` around lines 95 - 139, parseExtraction is repetitive; introduce a shared helper to encapsulate the "extract → parse → append" pattern and use it for parser.Ifcfg, parser.NM, parser.Interfaces and the generic single-section flows: create a helper (eg. parseAndAppend(output, start, end, parseFn)) that calls extractSection(output, start, end), returns early on empty section, calls the provided parseFn(section) which returns ([]api.Interface, error) and appends to guest.Interfaces or returns wrapped error; keep special handling for parser.Netplan and parser.Wicked outside or provide a small variant (eg. parseNetplan(output, start, end) to set guest.NetplanRenderer and parseWicked(output, filesStart, xmlStart, end) that calls parser.Wicked(wickedFiles, wickedXML)), then replace the duplicated blocks in parseExtraction with calls to the new helpers while preserving existing error messages (fmt.Errorf("parsing ...: %w", err)) and behavior.pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go (1)
19-78: ⚡ Quick winRun these tests in parallel.
These cases are independent and can safely use
t.Parallel()to improve test throughput.As per coding guidelines:
**/*_test.go— “Parallelism: ensure tests are independent as much as possible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go` around lines 19 - 78, Add t.Parallel() at the start of each independent test to run them in parallel: insert t.Parallel() as the first statement inside TestRenderComplementaryTemplate, TestRenderComplementaryTemplate_SingleQuoteEscaping, and TestRenderComplementaryTemplate_Empty (immediately after the function's t *testing.T parameter is available) so the tests execute concurrently while preserving their existing assertions and calls to RenderComplementaryTemplate.pkg/virt-v2v/customization/probe/parser/nm_test.go (1)
5-139: ⚡ Quick winParallelize these unit tests.
Each case is self-contained and can call
t.Parallel()to improve runtime and strengthen isolation guarantees.As per coding guidelines:
**/*_test.go— “Parallelism: ensure tests are independent as much as possible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/nm_test.go` around lines 5 - 139, Add test parallelism by calling t.Parallel() at the start of each test function (e.g., TestNM, TestNM_DHCP, TestNM_MultiProfile, TestNM_IPv6, TestNM_NoConnectionHeader, TestNM_NonIPSectionsIgnoreAddresses, TestNM_EmptyInput); insert the call as the first statement in each test to enable Go's test runner to run them concurrently and ensure no shared mutable state is introduced between tests.pkg/virt-v2v/customization/probe/parser/ifcfg_test.go (1)
5-142: ⚡ Quick winAdd
t.Parallel()to independent test cases.These tests are hermetic and independent, so running them in parallel will speed up CI and catch hidden shared-state coupling earlier.
As per coding guidelines:
**/*_test.go— “Parallelism: ensure tests are independent as much as possible.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/ifcfg_test.go` around lines 5 - 142, Add t.Parallel() to each independent test function (TestIfcfg, TestIfcfg_DHCP, TestIfcfg_NumberedIPADDR, TestIfcfg_IPv6, TestIfcfg_EmptyIPADDR, TestIfcfg_ConcatenatedRecords, TestIfcfg_EmptyInput) as the first statement inside the function body so they run in parallel; ensure it is placed before any setup or assertions and that no tests share mutable global state that would break parallel execution.pkg/virt-v2v/customization/customize.go (1)
80-80: ⚡ Quick winUse structured logging instead of
fmt.Printfin orchestration flow.Please switch this plugin-action log line to the project logger and include context fields (e.g., plugin name).
As per coding guidelines:
**/*.go— “Observability: Instrument code with structured logging… Logs should provide context, not just messages.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/customize.go` at line 80, Replace the fmt.Printf call that prints "Collecting actions from plugin: %s" with the project structured logger: remove fmt.Printf and use the package logger (e.g., logger or log) to emit an Info-level message with a plugin context field, e.g., logger.With("plugin", p.Name()).Info("Collecting actions from plugin"), referencing the existing p.Name() call and removing the raw fmt usage in customize.go so logs are structured and include the plugin name as a field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/virt-v2v/customization/commit/files.go`:
- Around line 80-89: The switch over action.Type inside the loop over actions
currently ignores unknown api.FileAction types; update the switch in the
function that builds the guestfish command buffer (the loop using variable
actions and buf) to add a default branch that returns a descriptive error (e.g.,
fmt.Errorf("unsupported file action type: %v", action.Type)) so the caller can
fail fast on unrecognized action types instead of silently skipping them; ensure
the function signature is adjusted to return an error (and propagate it where
this function is called) if it currently does not.
In `@pkg/virt-v2v/customization/customize_test.go`:
- Around line 167-397: The TestRun function is too large/complex; split it into
multiple focused top-level tests (e.g., TestRun_MissingConfig,
TestRun_EmptyDisks, TestRun_ProbeFailure, TestRun_PluginApplyError,
TestRun_FileActionsCommitFiles, TestRun_NoFileActionsNoCommit,
TestRun_ExecActionsCommitScripts, TestRun_CommitFilesError,
TestRun_CommitScriptsError, TestRun_LuksKeyResolutionError, TestRun_HappyPath)
by moving each t.Run case into its own TestXxx function that calls Run with
Options and asserts results; extract shared setup (noopFS, noopProbe,
noopCommitFiles, noopCommitScripts, stubPlugin) and common assertion helpers
into package-level helper functions to avoid duplication while keeping each test
small and focused.
- Around line 73-163: TestLuksKeys is too complex; refactor by extracting the
two assertion paths into small helpers: create assertLuksKeysError(t *testing.T,
fs utils.FileSystem, cfg *config.AppConfig, wantErr string) that calls luksKeys
and asserts the error contains wantErr, and create assertLuksKeysOK(t
*testing.T, fs utils.FileSystem, cfg *config.AppConfig, wantKeys []string) that
calls luksKeys, checks err==nil and compares result using slicesEqual; then
update TestLuksKeys to call these helpers inside each subtest (referring to
TestLuksKeys, luksKeys, slicesEqual) so the main test loop only selects which
helper to invoke, reducing cognitive complexity.
In `@pkg/virt-v2v/customization/plugins/network/registry/plugin.go`:
- Around line 37-45: Add nil guards to avoid panics when ctx or its fields are
absent: in Plugin.Applicable check for nil ctx, ctx.Guest and ctx.Config before
accessing ctx.Guest.OS or ctx.Config.StaticIPs and return false if any are nil;
similarly update Plugin.Apply to validate ctx, ctx.Guest and ctx.Config at the
top and return an error if any are nil. Touch both Applicable and Apply (the
functions referenced as Plugin.Applicable and Plugin.Apply covering the earlier
37-45 and later 47-83 sections) so all accesses to ctx.Guest and ctx.Config are
safe.
In
`@pkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1`:
- Around line 66-67: The script
pkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1
contains Unicode punctuation that triggers PSUseBOMForUnicodeEncodedFile; fix
this by saving the file as UTF-8 with a BOM (UTF-8-BOM) or by replacing any
non-ASCII punctuation characters (e.g., smart quotes, em-dashes) in the
comments/strings with ASCII equivalents so the file parses consistently across
environments—open 120_remove_duplicate_routes.ps1, convert encoding to UTF-8
with BOM (or perform the replacements), and commit the updated file.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render.go`:
- Around line 36-44: The lengthOf function currently returns 0 for unknown input
types which hides template/data mismatches; update the default branch in
lengthOf (function name lengthOf, handling []IPEntry and []IPConfig) to fail
loudly by panicking or returning an error-like fatal message that includes the
actual type (use fmt.Sprintf("%T", v) or similar) and context (e.g.,
"unsupported template input type in lengthOf"), so template mismatches are
surfaced instead of silently producing 0.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 141-151: The extractSection function can panic because it
unconditionally does startIdx += len(startMarker) + 1 and then slices
output[startIdx:], which is out of range if the output ends exactly at the
marker; change the logic in extractSection to advance startIdx by
len(startMarker) (remove the forced +1), then check if startIdx >= len(output)
and return "" if so, before doing any slicing or calling strings.Index on
output[startIdx:]; keep the rest of the flow (searching for endMarker and
returning the substring) intact so functions like extractSection and its callers
are safe on truncated probe output.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg.go`:
- Around line 17-76: The Ifcfg function mixes record-boundary detection, KV
parsing and field mapping in one loop which raises cognitive complexity;
refactor by extracting small helpers: add flushCurrent(current
*api.InterfaceInfo, interfaces *[]api.InterfaceInfo) to handle record completion
and appending, parseKV(line string) (key, val string, ok bool) to centralize
splitting and trimming, and applyKey(current *api.InterfaceInfo, key, val
string) to map keys to fields (handling DEVICE, NAME, HWADDR, BOOTPROTO,
IPV6ADDR*, IPADDR* etc.); then simplify the main loop in Ifcfg to only scan
lines, call parseKV, call applyKey, and call flushCurrent on blank lines or when
DEVICE appears, and perform a final flush after the loop, preserving existing
behavior and error handling (scanner.Err()).
In `@pkg/virt-v2v/customization/probe/parser/interfaces.go`:
- Around line 26-42: The parser currently treats "iface lo" as non-resetting,
causing subsequent address lines to bleed into the previous interface; modify
the iface handling in the block that checks fields[0] == "iface" so that when
fields[1] == "lo" you reset hasData and current (e.g., set hasData = false and
current = api.InterfaceInfo{Source: "interfaces"}) instead of populating the
previous entry, otherwise keep the existing logic of appending current to
interfaces when hasData is true and then setting current.Name, isV6 and
current.DHCP; ensure the subsequent address-handling branch (which uses hasData
and isV6 to append to current.IPv4/IPv6) will only apply when a non-loopback
iface has been started.
In `@pkg/virt-v2v/customization/resolve.go`:
- Around line 39-47: Resolve currently assumes ctx is non-nil and may panic when
calling p.Applicable(ctx); add a nil check at the top of Resolve (for the
api.Context parameter) and return an empty []api.Plugin{} immediately if ctx ==
nil, before calling AllPlugins() or invoking p.Applicable; keep function
signature unchanged and only modify Resolve to perform this input validation.
---
Outside diff comments:
In `@cmd/virt-v2v/entrypoint.go`:
- Around line 34-113: Write unit tests for cmd/virt-v2v/entrypoint.go that
exercise the main control-flow branches: when env.SkipConversion is true (no
conversion calls), when env.IsRemoteInspection is true (mock
convert.RunRemoteV2vInspection and assert it’s called and function returns
early), for in-place conversion with convert.LibvirtUrl set (mock
convert.GetDomainXML to return XML, ensure WriteFile path
convert.LibvirtDomainFile and convert.RunVirtV2vInPlace are invoked) and without
LibvirtUrl (mock RunVirtV2vInPlaceDisk), for the non-in-place path (mock
RunVirtV2v), ensure RunVirtV2VInspection is invoked after conversion, when
env.SkipCustomize is true vs false (mock convert.RunCustomize to return error
and assert server.AddWarning is called), and when convert.IsLocalMigration is
true (mock server.Server.Start to simulate success/failure). Use table-driven
tests and replace/spy the functions on the convert and server types
(RunRemoteV2vInspection, GetDomainXML, RunVirtV2vInPlace, RunVirtV2vInPlaceDisk,
RunVirtV2v, RunVirtV2VInspection, RunCustomize, and server.Server.Start) to
verify each branch and error handling.
---
Duplicate comments:
In `@pkg/virt-v2v/customization/probe/parser/wicked.go`:
- Around line 129-131: The DecodeElement call currently swallows errors and
continues, leading to silent failures; change the logic around
decoder.DecodeElement(&content, &t) so that if err != nil you wrap it with
context (mentioning the element/token and the interface being parsed) and return
the error instead of ignoring it; update the surrounding function (the parser
routine that declares decoder, t and address) to propagate that returned error
to callers so callers can handle or report XML decode failures.
---
Nitpick comments:
In `@pkg/virt-v2v/customization/customize.go`:
- Line 80: Replace the fmt.Printf call that prints "Collecting actions from
plugin: %s" with the project structured logger: remove fmt.Printf and use the
package logger (e.g., logger or log) to emit an Info-level message with a plugin
context field, e.g., logger.With("plugin", p.Name()).Info("Collecting actions
from plugin"), referencing the existing p.Name() call and removing the raw fmt
usage in customize.go so logs are structured and include the plugin name as a
field.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.go`:
- Around line 19-78: Add t.Parallel() at the start of each independent test to
run them in parallel: insert t.Parallel() as the first statement inside
TestRenderComplementaryTemplate,
TestRenderComplementaryTemplate_SingleQuoteEscaping, and
TestRenderComplementaryTemplate_Empty (immediately after the function's t
*testing.T parameter is available) so the tests execute concurrently while
preserving their existing assertions and calls to RenderComplementaryTemplate.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 95-139: parseExtraction is repetitive; introduce a shared helper
to encapsulate the "extract → parse → append" pattern and use it for
parser.Ifcfg, parser.NM, parser.Interfaces and the generic single-section flows:
create a helper (eg. parseAndAppend(output, start, end, parseFn)) that calls
extractSection(output, start, end), returns early on empty section, calls the
provided parseFn(section) which returns ([]api.Interface, error) and appends to
guest.Interfaces or returns wrapped error; keep special handling for
parser.Netplan and parser.Wicked outside or provide a small variant (eg.
parseNetplan(output, start, end) to set guest.NetplanRenderer and
parseWicked(output, filesStart, xmlStart, end) that calls
parser.Wicked(wickedFiles, wickedXML)), then replace the duplicated blocks in
parseExtraction with calls to the new helpers while preserving existing error
messages (fmt.Errorf("parsing ...: %w", err)) and behavior.
In `@pkg/virt-v2v/customization/probe/parser/ifcfg_test.go`:
- Around line 5-142: Add t.Parallel() to each independent test function
(TestIfcfg, TestIfcfg_DHCP, TestIfcfg_NumberedIPADDR, TestIfcfg_IPv6,
TestIfcfg_EmptyIPADDR, TestIfcfg_ConcatenatedRecords, TestIfcfg_EmptyInput) as
the first statement inside the function body so they run in parallel; ensure it
is placed before any setup or assertions and that no tests share mutable global
state that would break parallel execution.
In `@pkg/virt-v2v/customization/probe/parser/nm_test.go`:
- Around line 5-139: Add test parallelism by calling t.Parallel() at the start
of each test function (e.g., TestNM, TestNM_DHCP, TestNM_MultiProfile,
TestNM_IPv6, TestNM_NoConnectionHeader, TestNM_NonIPSectionsIgnoreAddresses,
TestNM_EmptyInput); insert the call as the first statement in each test to
enable Go's test runner to run them concurrently and ensure no shared mutable
state is introduced between tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 705ae51a-e0de-4b79-985c-77ce24df9924
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (137)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/customize_test.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/staticip/render.gopkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.gopkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/customize.go
- pkg/virt-v2v/customize/testdata/valid_config.yml
- .github/workflows/pull-request.yml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
2bb1af5 to
372c44e
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
pkg/virt-v2v/customization/probe/parser/interfaces.go (1)
26-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the previous interface before handling
iface lo.When a valid interface stanza is already active, the early loopback branch discards it instead of appending it first. This can drop NIC data if
iface loappears after a non-loopback stanza.Proposed minimal fix
if len(fields) >= 4 && fields[0] == "iface" { - if fields[1] == "lo" { - hasData = false - current = api.InterfaceInfo{Source: "interfaces"} - continue - } if hasData { interfaces = append(interfaces, current) current = api.InterfaceInfo{Source: "interfaces"} + hasData = false + } + if fields[1] == "lo" { + continue } current.Name = fields[1] isV6 = fields[2] == "inet6" current.DHCP = fields[3] == "dhcp" hasData = true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/interfaces.go` around lines 26 - 35, The loop handling interface stanzas in interfaces.go drops the active interface when encountering "iface lo"; update the branch that checks fields[1] == "lo" to first append the existing active interface if hasData is true (interfaces = append(interfaces, current)), then reset hasData and set current = api.InterfaceInfo{Source: "interfaces"}; this preserves any previously parsed NIC data instead of discarding it while still treating "iface lo" as a new/ignored stanza.
🧹 Nitpick comments (13)
pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go (1)
70-86: 💤 Low valueIndex-based ordering assumption is acceptable but brittle if ordering ever changes.
The loop assumes
actions.Files[i]corresponds 1:1 withdriverRemovalScripts[i]. The test title advertises "ordered guest paths", so this is intentional, but if the plugin later switches to a map iteration the failure will be hard to diagnose. Consider asserting on the set of guest paths in addition to position (e.g., collectactions.Files[*].GuestPathandContainElements(...)) to make the intent more robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go` around lines 70 - 86, The test for Apply currently assumes actions.Files is ordered to match driverRemovalScripts by index; make it robust by additionally asserting the set of guest paths regardless of order: collect GuestPath values from actions.Files (from the Apply call in the Describe block) and assert they contain all expected entries from driverRemovalScripts (e.g., using a ContainElements-style check) while retaining one positional assertion if you want to ensure ordering when applicable; reference the Apply function, the driverRemovalScripts slice, and actions.Files to locate where to change the assertions.pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.go (1)
34-47: 💤 Low valueOptional: extract the script filename into a const to avoid duplicate string literals.
"200_restore_config_legacy.ps1"appears at both lines 35 and 43. A package-levelconst scriptName = "200_restore_config_legacy.ps1"would keep the read and theGuestPathin lockstep if the script is ever renamed.♻️ Proposed refactor
+const scriptName = "200_restore_config_legacy.ps1" + func (p *Plugin) Apply(ctx *api.Context) (*api.Actions, error) { - content, err := readScript("200_restore_config_legacy.ps1") + content, err := readScript(scriptName) if err != nil { return nil, fmt.Errorf("read embedded script: %w", err) } return &api.Actions{ Files: []api.FileAction{{ Type: api.ActionWrite, - GuestPath: path.Join(api.WinFirstbootScriptsPath, "200_restore_config_legacy.ps1"), + GuestPath: path.Join(api.WinFirstbootScriptsPath, scriptName), Content: content, }}, }, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.go` around lines 34 - 47, Extract the repeated filename into a package-level constant (e.g., const scriptName = "200_restore_config_legacy.ps1") and replace the literal in Plugin.Apply where readScript("200_restore_config_legacy.ps1") is called and where path.Join(api.WinFirstbootScriptsPath, "200_restore_config_legacy.ps1") builds GuestPath; keep the constant near the top of plugin.go and import/usage unchanged so readScript(scriptName) and path.Join(api.WinFirstbootScriptsPath, scriptName) are used.pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.go (1)
42-52: ⚡ Quick winTrailing/empty DNS tokens are kept verbatim — confirm downstream templates tolerate them.
ipParts[3:]is assigned toDNSas-is. For inputs like"...,8.8.8.8,"(a trailing comma is used in several test fixtures in this PR),DNSwill end up as["8.8.8.8", ""]. If the registry templates iterateDNSand emit a registry value per entry, an empty string could produce a malformedNameServerline. Consider filtering empty tokens here so the parser exposes a clean slice to all consumers.♻️ Proposed change
+ dns := make([]string, 0, len(ipParts)-3) + for _, d := range ipParts[3:] { + if d = strings.TrimSpace(d); d != "" { + dns = append(dns, d) + } + } macMap[mac] = append(macMap[mac], IPEntry{ IP: ipParts[0], Gateway: ipParts[1], PrefixLength: ipParts[2], - DNS: ipParts[3:], + DNS: dns, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.go` around lines 42 - 52, The DNS slice currently assigns raw tokens ipParts[3:] to IPEntry.DNS which preserves empty/trailing tokens (e.g., ["8.8.8.8",""]), so update the parsing in the ParseEntries block that builds macMap (the macMap[mac] = append(... IPEntry{...}) assignment) to filter and trim ipParts[3:]: create a new slice of non-empty, trimmed strings and assign that to DNS (so IPEntry.DNS contains only valid nameserver entries); keep the error/warning behavior unchanged and ensure any whitespace around tokens is trimmed.pkg/virt-v2v/customization/probe/extract.go (1)
95-139: ⚡ Quick winRefactor
parseExtractioninto per-source helpers to reduce branching.The function still centralizes five parse paths with repeated extract/parse/append logic, which keeps cognitive complexity high and harder to maintain. Splitting each source block into small helpers (or a table-driven dispatcher) would keep behavior unchanged and simplify future parser additions.
As per coding guidelines: Clarity and Simplicity: “Write idiomatic Go. Code should be self-documenting. Complexity should be justified.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/extract.go` around lines 95 - 139, The parseExtraction function centralizes repetitive extract/parse/append logic; refactor it by extracting each source block into small helper functions (e.g., parseIfcfg, parseNM, parseNetplan, parseInterfaces, parseWicked) that take the full output and the guest pointer, perform extractSection + the corresponding parser call (parser.Ifcfg, parser.NM, parser.Netplan, parser.Interfaces, parser.Wicked), handle errors, and append to guest.Interfaces / set guest.NetplanRenderer as appropriate; then replace the inline blocks in parseExtraction with calls to these helpers (or a table-driven dispatcher) to preserve exact behavior and error returns while reducing branching and duplication.pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.go (1)
30-32: ⚡ Quick winAdd nil guards to
Applicable(andApply) to avoid panics on incomplete context.
Applicabledereferencesctx.Guest.OSandctx.Configwithout checking for nil. If any caller invokes the plugin with a partially constructed*api.Context, this will panic. The siblingregistryplugin in this PR already follows the safer pattern of guardingctx,ctx.Guest, andctx.Config— apply the same convention here for consistency and robustness.Suggested change
func (p *Plugin) Applicable(ctx *api.Context) bool { + if ctx == nil || ctx.Guest == nil || ctx.Config == nil { + return false + } return ctx.Guest.OS.IsWindows() && ctx.Config.WaitForGuestReboot }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.go` around lines 30 - 32, Add nil-checks in Plugin.Applicable and Plugin.Apply to avoid panics when context is incomplete: ensure ctx, ctx.Guest, ctx.Guest.OS and ctx.Config are non-nil before dereferencing them. Update Applicable to return false if any of those are nil, and mirror the same defensive checks at the start of Apply so it safely handles partially constructed *api.Context values (refer to method names Plugin.Applicable and Plugin.Apply for locating the changes).pkg/virt-v2v/customization/plugins/network/registry/plugin.go (1)
114-117: ⚡ Quick winUse structured logging instead of
fmt.Printffor parse warnings.
fmt.Printf("Warning: %v\n", w)writes to stdout unstructured. Per project observability conventions, parse warnings should be logged via the project's logger (e.g.,klog/logr) with structured key/value pairs so they can be filtered and correlated. Stdout in this binary is also being fed into guestfish/cmd pipelines elsewhere — mixing free‑form warnings into stdout risks noise.As per coding guidelines: "Observability: Instrument code with structured logging, metrics (e.g., Prometheus), and tracing. Logs should provide context, not just messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/plugin.go` around lines 114 - 117, Replace the unstructured stdout write in the ParseEntries handling with the project's structured logger: capture the returned warnings from staticip.ParseEntries(staticIPs) and for each warning call the module logger (e.g., klog.V(...).InfoS or a provided logr.Logger) instead of fmt.Printf, including structured key/value pairs such as "component":"network-plugin", "warning": w, and any relevant context like "staticIPs": staticIPs or "mac": macMap entry to allow filtering and correlation; update imports to include the chosen logger if not already present and remove the fmt.Printf usage referencing warnings, macMap, and staticip.ParseEntries.pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go (1)
7-116: 💤 Low valueConsider enabling
t.Parallel()for these hermetic unit tests.These tests have no shared state and are pure unit tests of
ParseEntries/BuildComplementaryConfigs. Addingt.Parallel()in each function aligns with the team's preference for test parallelism on independent unit tests.As per coding guidelines: "Parallelism: ensure tests are independent as much as possible."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go` around lines 7 - 116, These unit tests are hermetic and should run in parallel: in each test function (TestParseEntries_SingleMAC, TestParseEntries_MultipleIPs, TestParseEntries_SkipsMalformed, TestBuildComplementaryConfigs_DropsFirstIP, TestBuildComplementaryConfigs_SingleIPSkipped, TestBuildComplementaryConfigs_SortedByMAC) add a call to t.Parallel() as the first statement inside the function body so the tests can safely run concurrently; ensure you place it before any setup to keep tests independent.pkg/virt-v2v/customization/commit/files.go (2)
57-100: ⚡ Quick winExtract mkdir‑p emission to a helper to reduce cognitive complexity.
SonarCloud flags
buildScriptat cognitive complexity 17 (limit 15). The dedupe/sort/emitmkdir-pblock (lines 60–78) is self‑contained and trivially extractable into something likeemitMkdirs(&buf, actions). That keeps the main loop focused on per‑action emission.As per coding guidelines: "Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/commit/files.go` around lines 57 - 100, buildScript is over the cognitive complexity limit because the mkdir-p dedupe/sort/emit block is embedded; extract that block into a small helper (e.g., emitMkdirs(buf *bytes.Buffer, actions []api.FileAction) or emitMkdirs(buf *bytes.Buffer, dirs map[string]bool)) and call it from buildScript before the per-action loop. The helper should: collect parent dirs from each action.GuestPath (same path.Dir logic and filtering), dedupe into a map, sort the keys, and write the lines using fmt.Fprintf(&buf, "mkdir-p %s\n", strconv.Quote(d)) — preserving existing behavior and using the same imports and types so buildScript only calls emitMkdirs(&buf, actions) and then proceeds to emit per-action lines.
17-17: 💤 Low valueOctal mode regex rejects 4‑digit modes with special bits.
^0?[0-7]{3}$accepts755and0755but rejects1755(sticky),2755(setgid),4755(setuid), and other 4‑digit modes whose leading digit is not0. If any plugin ever needs to set such bits,buildScriptwill fail with "invalid permissions". Consider broadening to^[0-7]{3,4}$to match standard chmod octal modes.Suggested change
-var validOctalMode = regexp.MustCompile(`^0?[0-7]{3}$`) +var validOctalMode = regexp.MustCompile(`^[0-7]{3,4}$`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/commit/files.go` at line 17, The current regex variable validOctalMode (`^0?[0-7]{3}$`) rejects 4‑digit modes with special bits (e.g., 1755/4755); change the pattern to accept either 3 or 4 octal digits by replacing it with `^[0-7]{3,4}$` so modes like 755, 0755, 1755, 2755, 4755 are all allowed; update the definition of validOctalMode in commit/files.go and ensure any validation logic using validOctalMode (e.g., buildScript or related permission checks) continues to use this updated variable.pkg/virt-v2v/customization/probe/probe_test.go (2)
12-112: 💤 Low valueConsider consolidating the
parseDetectioncases into a table‑driven test.The ten
TestParseDetection_*functions share the same shape: build a 10‑lineoutput, invokeparseDetection, and assert one or twoguestfields. A single table‑driven test would reduce duplication, make the index↔check‑name mapping explicit in one place, and make it easier to extend when probes are added.As per coding guidelines: "Readability: Use table-driven tests for multiple cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/probe_test.go` around lines 12 - 112, Multiple nearly-identical tests for parseDetection should be consolidated into a single table-driven test: create a TestParseDetection_TableDriven that defines a slice of cases (with name, output string, and expected fields such as OS.Family and booleans UsesIfcfg, UsesNetworkManager, UsesIfcfgSuse, UsesNetplan, UsesIfquery, UsesInterfacesD, UsesWicked), loop over cases invoking parseDetection(output, guest) and run subtests (t.Run) asserting the expected fields; remove the ten TestParseDetection_* functions and keep the individual expectations expressed in the table so adding probes later only requires adding rows to the table.
12-191: 💤 Low valueAdd
t.Parallel()to these hermetic tests.All tests in this file are pure unit tests with no shared state. Adding
t.Parallel()at the top of each (or in the table‑driven version) shortens test runtime and matches the convention used infiles_test.goin this same PR.As per coding guidelines: "Parallelism: ensure tests are independent as much as possible."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/probe_test.go` around lines 12 - 191, Add t.Parallel() to the start of each hermetic test function (e.g. TestParseDetection_Windows, TestParseDetection_Linux, TestParseDetection_IfcfgSuse, TestParseDetection_NetworkManager, TestParseDetection_Netplan, TestParseDetection_Ifquery, TestParseDetection_InterfacesD, TestParseDetection_Wicked_EtcWicked, TestParseDetection_Wicked_VarLibWicked, TestParseDetection_Unknown, TestParseOsRelease_RHEL, TestParseOsRelease_Ubuntu, TestBuildExtractionScript_IfcfgOnly, TestBuildExtractionScript_Windows, TestBuildExtractionScript_MultipleStacks, TestExtractSection_Present, TestExtractSection_Missing) by inserting t.Parallel() immediately after the function starts (right after the t *testing.T parameter is available) so each unit test runs in parallel; do not change test logic or introduce shared mutable state.pkg/virt-v2v/customization/plugins/network/udev/plugin.go (1)
23-25: ⚡ Quick winAdd nil guards to
Applicable.
Applicabledereferencesctx.Guest.OS,ctx.Config.StaticIPs, andctx.Guest.Interfaceswithout checking for nil. Be defensive: if any caller passes a partially constructed*api.Context, this panics. Theregistryplugin in this PR already guardsctx,ctx.Guest, andctx.Config; apply the same pattern here (and inApply) for consistency.Suggested change
func (p *Plugin) Applicable(ctx *api.Context) bool { + if ctx == nil || ctx.Guest == nil || ctx.Config == nil { + return false + } return ctx.Guest.OS.IsLinux() && ctx.Config.StaticIPs != "" && len(ctx.Guest.Interfaces) > 0 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go` around lines 23 - 25, Add defensive nil guards in Plugin.Applicable and Plugin.Apply: check ctx, ctx.Guest, ctx.Guest.OS, ctx.Config and ctx.Guest.Interfaces for nil before dereferencing; have Applicable return false if any are nil, and have Apply perform the same guards and return early (no-op) or a controlled error instead of dereferencing. Update the checks around ctx.Guest.OS.IsLinux(), ctx.Config.StaticIPs, and len(ctx.Guest.Interfaces) to only run after these nil checks so neither Applicable nor Apply can panic when given a partially constructed *api.Context.pkg/virt-v2v/customization/commit/files_test.go (1)
10-193: ⚡ Quick winAdd negative-path tests for
buildScripterror returns.The newly added validation paths in
buildScript— unsupportedaction.Type(line 90 infiles.go) and invalidPermissionsrejected byvalidOctalMode(lines 93–95) — currently have no test coverage. Adding two small tests that assert non‑nil errors and a meaningful message would lock in the input‑validation contract.Suggested additions
func TestBuildScript_UnsupportedActionType(t *testing.T) { t.Parallel() _, err := buildScript([]api.FileAction{{Type: "bogus", GuestPath: "/x"}}) if err == nil { t.Fatal("expected error for unsupported action type") } } func TestBuildScript_InvalidPermissions(t *testing.T) { t.Parallel() _, err := buildScript([]api.FileAction{{ Type: api.ActionWrite, GuestPath: "/x", Content: []byte("x"), Permissions: "abc", }}) if err == nil { t.Fatal("expected error for invalid permissions") } }As per coding guidelines: "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/commit/files_test.go` around lines 10 - 193, Add negative-path unit tests for buildScript to cover the new validation branches: create TestBuildScript_UnsupportedActionType that calls buildScript with an action having Type "bogus" and asserts a non-nil error (and a message mentioning unsupported action/type), and TestBuildScript_InvalidPermissions that calls buildScript with a write action whose Permissions is "abc" and asserts a non-nil error (and a message indicating invalid permissions); reference buildScript and validOctalMode in the tests so they exercise the unsupported action.Type branch and the permissions validation branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/virt-v2v/customization/commit/scripts.go`:
- Around line 13-23: The function Scripts currently proceeds to build and run
virt-customize even when the disks slice is empty; add an explicit input
validation at the start of Scripts to fail fast when no disks are provided
(e.g., if len(disks) == 0) and return a clear error (using fmt.Errorf or
similar) indicating "no disks provided for virt-customize"; place this guard
before any use of cmdBuilder.New("virt-customize") so callers get immediate,
descriptive feedback instead of an external failure path.
In `@pkg/virt-v2v/customization/customize.go`:
- Around line 103-110: The code calls doScripts(opts.CommandBuilder, opts.Disks,
keys, allExecs) unconditionally even when allExecs is empty; guard this call so
virt-customize is only invoked if there are exec actions to run. Update the
block around doScripts/commit.Scripts/opts.commitScripts to check len(allExecs)
> 0 (or equivalent) before calling doScripts with opts.CommandBuilder,
opts.Disks, keys, allExecs, and skip the call (and return nil) when there are no
execs to perform.
In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin.go`:
- Around line 40-45: The Applicable method on Plugin dereferences ctx and nested
fields without validation; update Plugin.Applicable to first nil-check ctx and
then defensively check ctx.Guest, ctx.Guest.OS and ctx.Config before calling
methods or accessing fields (e.g., ctx.Guest.OS.IsWindows(),
ctx.Config.VsphereVmwareDriverRemoval, ctx.Config.Source == config.VSPHERE),
returning false if any required pointer is nil so the function never panics on
direct plugin calls.
In
`@pkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1`:
- Around line 66-77: The guard incorrectly treats per-route activity as
per-interface activity; replace the $activeCount loop with logic that computes
the count of unique active interfaces for the $routes group (e.g., collect
unique InterfaceIndex values from $routes, test each unique InterfaceIndex
against Get-NetAdapter) and compare that unique-active-interface count to the
total number of unique interfaces in the group before skipping; update
references to $activeCount, $routes, and the group skip message (group.Name) to
reflect the unique-interface count check.
In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go`:
- Around line 27-44: The Apply method currently aborts on the first malformed
StaticIPs entry; change it to mirror the registry plugin's behavior by skipping
malformed pairs with a structured warning instead of returning an error: in
Plugin.Apply (and around parseStaticIPs / loop over pairs) validate MACs and IPs
as you do now but on validation failure call the context logger (e.g.,
ctx.Logger or the same logging facility used by the registry plugin) to emit a
clear warning mentioning the invalid token and continue to the next pair; ensure
any wrapped errors include context (invalid MAC/IP in StaticIPs + the offending
value) when you do need to return an error for irrecoverable failures, but do
not abort the whole Apply for a single bad pair.
In `@pkg/virt-v2v/customization/probe/detect.go`:
- Around line 35-43: parseDetection currently silently tolerates truncated
guestfish output by filling missing detectionChecks entries as false; update
parseDetection to detect when len(lines) < len(detectionChecks) and emit a
structured warning including expected=len(detectionChecks) and
received=len(lines) plus a guest identifier from guest (e.g., guest.ID or
guest.Name) so operators can see incomplete probe runs, then continue populating
results as before; reference parseDetection, detectionChecks, lines, results and
guest to locate where to add the warning log.
In `@pkg/virt-v2v/customization/probe/probe_test.go`:
- Around line 193-204: Remove the custom substring helpers strContains and
findSubstr and replace their uses with the standard library function
strings.Contains; update the test file to import "strings", remove both helper
functions (strContains, findSubstr), and change any assertions or calls that use
strContains(...) to strings.Contains(...). Ensure imports are tidied so the
"strings" package is included only if needed.
---
Duplicate comments:
In `@pkg/virt-v2v/customization/probe/parser/interfaces.go`:
- Around line 26-35: The loop handling interface stanzas in interfaces.go drops
the active interface when encountering "iface lo"; update the branch that checks
fields[1] == "lo" to first append the existing active interface if hasData is
true (interfaces = append(interfaces, current)), then reset hasData and set
current = api.InterfaceInfo{Source: "interfaces"}; this preserves any previously
parsed NIC data instead of discarding it while still treating "iface lo" as a
new/ignored stanza.
---
Nitpick comments:
In `@pkg/virt-v2v/customization/commit/files_test.go`:
- Around line 10-193: Add negative-path unit tests for buildScript to cover the
new validation branches: create TestBuildScript_UnsupportedActionType that calls
buildScript with an action having Type "bogus" and asserts a non-nil error (and
a message mentioning unsupported action/type), and
TestBuildScript_InvalidPermissions that calls buildScript with a write action
whose Permissions is "abc" and asserts a non-nil error (and a message indicating
invalid permissions); reference buildScript and validOctalMode in the tests so
they exercise the unsupported action.Type branch and the permissions validation
branch.
In `@pkg/virt-v2v/customization/commit/files.go`:
- Around line 57-100: buildScript is over the cognitive complexity limit because
the mkdir-p dedupe/sort/emit block is embedded; extract that block into a small
helper (e.g., emitMkdirs(buf *bytes.Buffer, actions []api.FileAction) or
emitMkdirs(buf *bytes.Buffer, dirs map[string]bool)) and call it from
buildScript before the per-action loop. The helper should: collect parent dirs
from each action.GuestPath (same path.Dir logic and filtering), dedupe into a
map, sort the keys, and write the lines using fmt.Fprintf(&buf, "mkdir-p %s\n",
strconv.Quote(d)) — preserving existing behavior and using the same imports and
types so buildScript only calls emitMkdirs(&buf, actions) and then proceeds to
emit per-action lines.
- Line 17: The current regex variable validOctalMode (`^0?[0-7]{3}$`) rejects
4‑digit modes with special bits (e.g., 1755/4755); change the pattern to accept
either 3 or 4 octal digits by replacing it with `^[0-7]{3,4}$` so modes like
755, 0755, 1755, 2755, 4755 are all allowed; update the definition of
validOctalMode in commit/files.go and ensure any validation logic using
validOctalMode (e.g., buildScript or related permission checks) continues to use
this updated variable.
In `@pkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.go`:
- Around line 30-32: Add nil-checks in Plugin.Applicable and Plugin.Apply to
avoid panics when context is incomplete: ensure ctx, ctx.Guest, ctx.Guest.OS and
ctx.Config are non-nil before dereferencing them. Update Applicable to return
false if any of those are nil, and mirror the same defensive checks at the start
of Apply so it safely handles partially constructed *api.Context values (refer
to method names Plugin.Applicable and Plugin.Apply for locating the changes).
In `@pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.go`:
- Around line 34-47: Extract the repeated filename into a package-level constant
(e.g., const scriptName = "200_restore_config_legacy.ps1") and replace the
literal in Plugin.Apply where readScript("200_restore_config_legacy.ps1") is
called and where path.Join(api.WinFirstbootScriptsPath,
"200_restore_config_legacy.ps1") builds GuestPath; keep the constant near the
top of plugin.go and import/usage unchanged so readScript(scriptName) and
path.Join(api.WinFirstbootScriptsPath, scriptName) are used.
In `@pkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.go`:
- Around line 70-86: The test for Apply currently assumes actions.Files is
ordered to match driverRemovalScripts by index; make it robust by additionally
asserting the set of guest paths regardless of order: collect GuestPath values
from actions.Files (from the Apply call in the Describe block) and assert they
contain all expected entries from driverRemovalScripts (e.g., using a
ContainElements-style check) while retaining one positional assertion if you
want to ensure ordering when applicable; reference the Apply function, the
driverRemovalScripts slice, and actions.Files to locate where to change the
assertions.
In `@pkg/virt-v2v/customization/plugins/network/registry/plugin.go`:
- Around line 114-117: Replace the unstructured stdout write in the ParseEntries
handling with the project's structured logger: capture the returned warnings
from staticip.ParseEntries(staticIPs) and for each warning call the module
logger (e.g., klog.V(...).InfoS or a provided logr.Logger) instead of
fmt.Printf, including structured key/value pairs such as
"component":"network-plugin", "warning": w, and any relevant context like
"staticIPs": staticIPs or "mac": macMap entry to allow filtering and
correlation; update imports to include the chosen logger if not already present
and remove the fmt.Printf usage referencing warnings, macMap, and
staticip.ParseEntries.
In
`@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.go`:
- Around line 7-116: These unit tests are hermetic and should run in parallel:
in each test function (TestParseEntries_SingleMAC, TestParseEntries_MultipleIPs,
TestParseEntries_SkipsMalformed, TestBuildComplementaryConfigs_DropsFirstIP,
TestBuildComplementaryConfigs_SingleIPSkipped,
TestBuildComplementaryConfigs_SortedByMAC) add a call to t.Parallel() as the
first statement inside the function body so the tests can safely run
concurrently; ensure you place it before any setup to keep tests independent.
In `@pkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.go`:
- Around line 42-52: The DNS slice currently assigns raw tokens ipParts[3:] to
IPEntry.DNS which preserves empty/trailing tokens (e.g., ["8.8.8.8",""]), so
update the parsing in the ParseEntries block that builds macMap (the macMap[mac]
= append(... IPEntry{...}) assignment) to filter and trim ipParts[3:]: create a
new slice of non-empty, trimmed strings and assign that to DNS (so IPEntry.DNS
contains only valid nameserver entries); keep the error/warning behavior
unchanged and ensure any whitespace around tokens is trimmed.
In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go`:
- Around line 23-25: Add defensive nil guards in Plugin.Applicable and
Plugin.Apply: check ctx, ctx.Guest, ctx.Guest.OS, ctx.Config and
ctx.Guest.Interfaces for nil before dereferencing; have Applicable return false
if any are nil, and have Apply perform the same guards and return early (no-op)
or a controlled error instead of dereferencing. Update the checks around
ctx.Guest.OS.IsLinux(), ctx.Config.StaticIPs, and len(ctx.Guest.Interfaces) to
only run after these nil checks so neither Applicable nor Apply can panic when
given a partially constructed *api.Context.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 95-139: The parseExtraction function centralizes repetitive
extract/parse/append logic; refactor it by extracting each source block into
small helper functions (e.g., parseIfcfg, parseNM, parseNetplan,
parseInterfaces, parseWicked) that take the full output and the guest pointer,
perform extractSection + the corresponding parser call (parser.Ifcfg, parser.NM,
parser.Netplan, parser.Interfaces, parser.Wicked), handle errors, and append to
guest.Interfaces / set guest.NetplanRenderer as appropriate; then replace the
inline blocks in parseExtraction with calls to these helpers (or a table-driven
dispatcher) to preserve exact behavior and error returns while reducing
branching and duplication.
In `@pkg/virt-v2v/customization/probe/probe_test.go`:
- Around line 12-112: Multiple nearly-identical tests for parseDetection should
be consolidated into a single table-driven test: create a
TestParseDetection_TableDriven that defines a slice of cases (with name, output
string, and expected fields such as OS.Family and booleans UsesIfcfg,
UsesNetworkManager, UsesIfcfgSuse, UsesNetplan, UsesIfquery, UsesInterfacesD,
UsesWicked), loop over cases invoking parseDetection(output, guest) and run
subtests (t.Run) asserting the expected fields; remove the ten
TestParseDetection_* functions and keep the individual expectations expressed in
the table so adding probes later only requires adding rows to the table.
- Around line 12-191: Add t.Parallel() to the start of each hermetic test
function (e.g. TestParseDetection_Windows, TestParseDetection_Linux,
TestParseDetection_IfcfgSuse, TestParseDetection_NetworkManager,
TestParseDetection_Netplan, TestParseDetection_Ifquery,
TestParseDetection_InterfacesD, TestParseDetection_Wicked_EtcWicked,
TestParseDetection_Wicked_VarLibWicked, TestParseDetection_Unknown,
TestParseOsRelease_RHEL, TestParseOsRelease_Ubuntu,
TestBuildExtractionScript_IfcfgOnly, TestBuildExtractionScript_Windows,
TestBuildExtractionScript_MultipleStacks, TestExtractSection_Present,
TestExtractSection_Missing) by inserting t.Parallel() immediately after the
function starts (right after the t *testing.T parameter is available) so each
unit test runs in parallel; do not change test logic or introduce shared mutable
state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5baffef3-9aa5-4d2b-a92e-b4a55ea17401
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (137)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/customize_test.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/staticip/render.gopkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.gopkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- .github/workflows/pull-request.yml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/testdata/valid_config.yml
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/customize.go
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
372c44e to
d1c16e2
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
✅ Actions performedFull review triggered. |
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
d1c16e2 to
e74f701
Compare
Member
Author
|
@coderabbitai, hi, addressed the issue raised in the last review, can you re review? |
|
(ʘ‿ʘ)ノ🐇 ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.go (1)
33-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd nil context guards in both plugin entrypoints.
Line 34 and Line 39 dereference nested context fields without checks; a partial context can panic this plugin.
Suggested fix
func (p *Plugin) Applicable(ctx *api.Context) bool { + if ctx == nil || ctx.Guest == nil || ctx.Config == nil { + return false + } return ctx.Guest.OS.IsWindows() } func (p *Plugin) Apply(ctx *api.Context) (*api.Actions, error) { + if ctx == nil || ctx.Guest == nil || ctx.Config == nil { + return nil, fmt.Errorf("firstboot plugin: nil context, guest, or config") + } script := "900_firstboot_init.bat" if ctx.Config.VirtIoWinLegacyDrivers != "" { script = "900_firstboot_init_legacy.bat" }As per coding guidelines, "API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.go` around lines 33 - 40, Both Plugin entrypoints dereference nested ctx fields without nil checks and can panic; add guard checks at the start of Applicable and Apply to validate ctx and nested fields (e.g., ctx.Guest, ctx.Guest.OS, ctx.Config) before using them. In Applicable (method Plugin.Applicable) return false if ctx or ctx.Guest or ctx.Guest.OS is nil; in Apply (method Plugin.Apply) return a sensible default (nil, nil or an error consistent with callers) if ctx, ctx.Config, or ctx.Guest are nil before inspecting ctx.Config.VirtIoWinLegacyDrivers or ctx.Guest.OS. Ensure the checks short-circuit to avoid any dereference of ctx.* fields.
🧹 Nitpick comments (3)
pkg/virt-v2v/customization/api/types.go (1)
60-79: ⚡ Quick winAdd focused unit tests for exported lookup helpers.
Please add targeted tests for
GuestInfo.InterfaceForIPandInterfaceInfo.HasIPs(IPv4 hit, IPv6 hit, miss, empty slices) to lock behavior on this new API surface.As per coding guidelines:
**/*.go— “coverage: Make sure that the code has unit tests.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/api/types.go` around lines 60 - 79, Add unit tests that exercise GuestInfo.InterfaceForIP and InterfaceInfo.HasIPs across IPv4 hit, IPv6 hit, miss, and empty-slice cases: create a GuestInfo with multiple InterfaceInfo entries (populate Name, IPv4, IPv6) and assert InterfaceForIP returns the correct interface name for a matching IPv4 address, for a matching IPv6 address, and returns empty string for a non-matching IP; also add tests for InterfaceInfo.HasIPs that verify true when IPv4 or IPv6 slices are non-empty and false when both are empty. Use table-driven tests and the exported symbols GuestInfo.InterfaceForIP and InterfaceInfo.HasIPs so behavior is locked and coverage requirements are met.pkg/virt-v2v/customization/probe/parser/wicked_test.go (1)
5-162: ⚡ Quick winRun these independent tests in parallel.
These tests look hermetic and can run concurrently; adding
t.Parallel()at the start of each test improves suite runtime and enforces independence.As per coding guidelines, “Parallelism: ensure tests are independent as much as possible.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/probe/parser/wicked_test.go` around lines 5 - 162, Add t.Parallel() to the start of each test to allow them to run concurrently: insert it at the top of TestWicked, TestWicked_IPv6, TestWicked_NoLeaseFiles, TestParseLeaseFilenames, TestWicked_MoreFilesThanXML, TestWicked_EmptyInput, TestWicked_MalformedXML, and TestWicked_TruncatedXML so each test begins by calling t.Parallel() and remains hermetic (ensure no shared global state is accessed concurrently).pkg/virt-v2v/customization/plugins/network/udev/plugin.go (1)
39-44: ⚡ Quick winUse structured plugin logging instead of writing directly to stderr.
The warning path here bypasses the plugin/context logger, which makes correlation and filtering harder during conversion troubleshooting. Please emit these warnings through the shared structured logger with fields for the invalid token.
As per coding guidelines, “Observability: Instrument code with structured logging, metrics (e.g., Prometheus), and tracing. Logs should provide context, not just messages.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go` around lines 39 - 44, The warnings currently written with fmt.Fprintf(os.Stderr, ...) in the StaticIPs validation should use the plugin's structured logger instead; replace both stderr calls in pkg/virt-v2v/customization/plugins/network/udev/plugin.go with the shared structured logger (e.g., p.Log or plugin.Log) and emit warning-level logs that include fields for the invalid value (pair.MAC or pair.IP) and the parse error where present (err) so the messages become structured and queryable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/virt-v2v/customization/commit/scripts.go`:
- Around line 13-48: Add unit tests for the Scripts function covering the guard
and mapping paths: create a fake utils.CommandBuilder/Command that records added
args/flags and lets you simulate Run errors/success; write tests that (1) call
Scripts with empty actions and assert no error and no command built, (2) call
with empty disks and assert the validation error about "no disks provided for
virt-customize", (3) pass an api.ExecAction with an unsupported Type and assert
the returned error contains the unsupported type/index/value message, and (4)
pass valid api.ActionFirstboot and api.ActionRun actions and assert the built
command includes "--firstboot <value>" and "--run <value>" plus the "-a <disk>"
and "--key <key>" flags and that a successful Run returns nil. Ensure tests
reference Scripts, utils.CommandBuilder, and api.ExecAction to locate the code
under test.
In `@pkg/virt-v2v/customization/plugins/network/netsh/plugin.go`:
- Around line 36-47: The Applicable and Apply methods on Plugin currently
dereference ctx and ctx.Config (and nested fields like ctx.Guest) without nil
checks; add early nil guards to both functions: return false (or
no-op/appropriate error) if ctx == nil or ctx.Config == nil or ctx.Guest == nil
before accessing their fields, and update checks that read ctx.Config.StaticIPs,
ctx.Config.WindowsRegistryNetworkConfig, ctx.Config.VirtIoWinLegacyDrivers (and
any similar accesses in Apply) to only run after those nil guards so the methods
never panic when given a partial context.
In `@pkg/virt-v2v/customization/plugins/network/registry/plugin.go`:
- Around line 117-120: The code currently prints raw parse warnings from
staticip.ParseEntries using fmt.Printf which leaks MAC/IP data and bypasses
structured logging; replace the fmt.Printf loop so that the warnings from
staticip.ParseEntries (variable warnings) are not written to stdout but instead
sent to the component logger (e.g., use the plugin's logger or processLogger)
with structured context and/or redaction: map each warning to a non-sensitive
summary (e.g., count or redacted message) and log that via the existing logger
API, referencing macMap, warnings, and the staticip.ParseEntries call to locate
the change in plugin.go.
In `@pkg/virt-v2v/customization/probe/extract.go`:
- Around line 103-104: parseOsRelease currently swallows scanner errors; change
its signature to return an error, check scanner.Err() after the scan loop and
return a wrapped error on failure, and update all call sites (the
parseOsRelease(output, guest) at the call around the current line 103 and the
other call region around lines 169-182) to handle the returned error
(wrap/propagate or fail as appropriate). Specifically: modify parseOsRelease to
return error, ensure it returns errors for malformed/truncated input (including
scanner.Err()) with contextual messages, and replace the direct calls
parseOsRelease(output, guest) with error handling like if err :=
parseOsRelease(output, guest); err != nil { return fmt.Errorf("parsing
os-release: %w", err) } (or propagate the error up from the enclosing function).
In `@pkg/virt-v2v/customization/probe/parser/interfaces_test.go`:
- Around line 5-6: The test function TestInterfaces is independent and should
enable parallel execution; add a call to t.Parallel() at the start of
TestInterfaces (and similarly add t.Parallel() to the other independent test
referenced around lines 31-32) so both test functions run in parallel; locate
the TestInterfaces function and the other test function by name in the file and
insert t.Parallel() as the first statement inside each test body.
---
Duplicate comments:
In `@pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.go`:
- Around line 33-40: Both Plugin entrypoints dereference nested ctx fields
without nil checks and can panic; add guard checks at the start of Applicable
and Apply to validate ctx and nested fields (e.g., ctx.Guest, ctx.Guest.OS,
ctx.Config) before using them. In Applicable (method Plugin.Applicable) return
false if ctx or ctx.Guest or ctx.Guest.OS is nil; in Apply (method Plugin.Apply)
return a sensible default (nil, nil or an error consistent with callers) if ctx,
ctx.Config, or ctx.Guest are nil before inspecting
ctx.Config.VirtIoWinLegacyDrivers or ctx.Guest.OS. Ensure the checks
short-circuit to avoid any dereference of ctx.* fields.
---
Nitpick comments:
In `@pkg/virt-v2v/customization/api/types.go`:
- Around line 60-79: Add unit tests that exercise GuestInfo.InterfaceForIP and
InterfaceInfo.HasIPs across IPv4 hit, IPv6 hit, miss, and empty-slice cases:
create a GuestInfo with multiple InterfaceInfo entries (populate Name, IPv4,
IPv6) and assert InterfaceForIP returns the correct interface name for a
matching IPv4 address, for a matching IPv6 address, and returns empty string for
a non-matching IP; also add tests for InterfaceInfo.HasIPs that verify true when
IPv4 or IPv6 slices are non-empty and false when both are empty. Use
table-driven tests and the exported symbols GuestInfo.InterfaceForIP and
InterfaceInfo.HasIPs so behavior is locked and coverage requirements are met.
In `@pkg/virt-v2v/customization/plugins/network/udev/plugin.go`:
- Around line 39-44: The warnings currently written with fmt.Fprintf(os.Stderr,
...) in the StaticIPs validation should use the plugin's structured logger
instead; replace both stderr calls in
pkg/virt-v2v/customization/plugins/network/udev/plugin.go with the shared
structured logger (e.g., p.Log or plugin.Log) and emit warning-level logs that
include fields for the invalid value (pair.MAC or pair.IP) and the parse error
where present (err) so the messages become structured and queryable.
In `@pkg/virt-v2v/customization/probe/parser/wicked_test.go`:
- Around line 5-162: Add t.Parallel() to the start of each test to allow them to
run concurrently: insert it at the top of TestWicked, TestWicked_IPv6,
TestWicked_NoLeaseFiles, TestParseLeaseFilenames, TestWicked_MoreFilesThanXML,
TestWicked_EmptyInput, TestWicked_MalformedXML, and TestWicked_TruncatedXML so
each test begins by calling t.Parallel() and remains hermetic (ensure no shared
global state is accessed concurrently).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eaf295b3-f720-4e8d-b0dc-5156f8f8ebf2
⛔ Files ignored due to path filters (6)
docs/enhancements/customization-pipeline-plugins.mdis excluded by!**/*.mdpkg/virt-v2v/customization/commit/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/plugins/README.mdis excluded by!**/*.mdpkg/virt-v2v/customization/probe/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/firstboot/README.mdis excluded by!**/*.mdpkg/virt-v2v/customize/scripts/rhel/run/README.mdis excluded by!**/*.md
📒 Files selected for processing (137)
.github/workflows/pull-request.ymlcmd/virt-v2v/entrypoint.gopkg/virt-v2v/config/variables.gopkg/virt-v2v/conversion/conversion.gopkg/virt-v2v/customization/api/types.gopkg/virt-v2v/customization/commit/files.gopkg/virt-v2v/customization/commit/files_test.gopkg/virt-v2v/customization/commit/scripts.gopkg/virt-v2v/customization/customize.gopkg/virt-v2v/customization/customize_test.gopkg/virt-v2v/customization/detect_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin.gopkg/virt-v2v/customization/plugins/boot/conversiondone/plugin_test.gopkg/virt-v2v/customization/plugins/boot/conversiondone/scripts/990_signal_conversion_done.ps1pkg/virt-v2v/customization/plugins/boot/diskrestore/plugin.gopkg/virt-v2v/customization/plugins/boot/diskrestore/plugin_test.gopkg/virt-v2v/customization/plugins/boot/diskrestore/scripts/200_restore_config_legacy.ps1pkg/virt-v2v/customization/plugins/boot/firstboot/plugin.gopkg/virt-v2v/customization/plugins/boot/firstboot/plugin_test.gopkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init.batpkg/virt-v2v/customization/plugins/boot/firstboot/scripts/900_firstboot_init_legacy.batpkg/virt-v2v/customization/plugins/drivers/vmware/plugin.gopkg/virt-v2v/customization/plugins/drivers/vmware/plugin_test.gopkg/virt-v2v/customization/plugins/drivers/vmware/scripts/100_disable_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/101_remove_services.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/102_remove_drivers.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/103_query_registry.batpkg/virt-v2v/customization/plugins/drivers/vmware/scripts/104_remove_registry.batpkg/virt-v2v/customization/plugins/network/netsh/plugin.gopkg/virt-v2v/customization/plugins/network/netsh/plugin_test.gopkg/virt-v2v/customization/plugins/network/netsh/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/netsh/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/plugin.gopkg/virt-v2v/customization/plugins/network/registry/plugin_test.gopkg/virt-v2v/customization/plugins/network/registry/scripts/100_network_config.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/110_complementary_ips.ps1.tmplpkg/virt-v2v/customization/plugins/network/registry/scripts/120_remove_duplicate_routes.ps1pkg/virt-v2v/customization/plugins/network/registry/staticip/render.gopkg/virt-v2v/customization/plugins/network/registry/staticip/render_test.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip.gopkg/virt-v2v/customization/plugins/network/registry/staticip/staticip_test.gopkg/virt-v2v/customization/plugins/network/udev/plugin.gopkg/virt-v2v/customization/plugins/network/udev/plugin_test.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin.gopkg/virt-v2v/customization/plugins/scripts/dynamic/plugin_test.gopkg/virt-v2v/customization/probe/detect.gopkg/virt-v2v/customization/probe/extract.gopkg/virt-v2v/customization/probe/parser/ifcfg.gopkg/virt-v2v/customization/probe/parser/ifcfg_test.gopkg/virt-v2v/customization/probe/parser/interfaces.gopkg/virt-v2v/customization/probe/parser/interfaces_test.gopkg/virt-v2v/customization/probe/parser/netplan.gopkg/virt-v2v/customization/probe/parser/netplan_test.gopkg/virt-v2v/customization/probe/parser/nm.gopkg/virt-v2v/customization/probe/parser/nm_test.gopkg/virt-v2v/customization/probe/parser/wicked.gopkg/virt-v2v/customization/probe/parser/wicked_test.gopkg/virt-v2v/customization/probe/probe.gopkg/virt-v2v/customization/probe/probe_test.gopkg/virt-v2v/customization/resolve.gopkg/virt-v2v/customize/customize.gopkg/virt-v2v/customize/customize_test.gopkg/virt-v2v/customize/embed_tool.gopkg/virt-v2v/customize/mock_embed_tool.gopkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.leasepkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestampspkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xmlpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.networkpkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfacespkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/network_config_util.shpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnectionpkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rulepkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yamlpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.networkpkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIPpkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.shpkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmplpkg/virt-v2v/customize/testdata/valid_config.xmlpkg/virt-v2v/customize/testdata/valid_config.yml
💤 Files with no reviewable changes (78)
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/timestamps
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/test-network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth1.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth1-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/tmp/macToIP
- .github/workflows/pull-request.yml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-no-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-double-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/root/var/lib/dhclient/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/windows/9999-preserve_complementry_ips_per_nic.ps1.tmpl
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/etc/NetworkManager/system-connections/eth1 but with spaces.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-multi-ip-test.d/root/etc/NetworkManager/system-connections/ens32.nmconnection
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-single-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-17cdc369-2f93-4b93-9b65-603bddfd3c6d-eth0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-eth1.network
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/run/systemd/network/netplan-ens160.network
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens224
- pkg/virt-v2v/customize/scripts/rhel/run/network-interfaces-multiple-test.d/root/etc/network/interfaces.d/if-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/expected-udev.rule
- pkg/virt-v2v/customize/testdata/valid_config.xml
- pkg/virt-v2v/customize/testdata/valid_config.yml
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-1f2ea111-624b-32f6-8c93-12b2388516a6-enp1s0.lease
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-test.d/root/var/lib/NetworkManager/internal-6a0608c2-9d57-46d8-84d1-d2bbf6767acc-enp1s0.lease
- pkg/virt-v2v/customize/embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/etc/sysconfig/network-scripts/ifcfg-ens192
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth1
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-prefix-match-test.d/expected-udev.rule
- pkg/virt-v2v/customize/mock_embed_tool.go
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test-failure.d/expected-udev.rule
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-single-quotes-test.d/root/etc/sysconfig/network-scripts/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/systemd-test.d/root/etc/netplan/50-netplan.yaml
- pkg/virt-v2v/customize/customize.go
- pkg/virt-v2v/customize/customize_test.go
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-suse-test.d/root/etc/sysconfig/network/ifcfg-eth0
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-wicked-test.d/root/var/lib/wicked/lease-eth0-dhcp-ipv4.xml
- pkg/virt-v2v/customize/scripts/rhel/run/ifcfg-hwaddr-test.d/root/tmp/macToIP
- pkg/virt-v2v/customize/scripts/rhel/run/netplan-test.d/root/run/systemd/network/netplan-enp3s0.network
- pkg/virt-v2v/customize/scripts/rhel/run/network_config_util.sh
- pkg/virt-v2v/customize/scripts/rhel/run/dhcp-networkmanager-dhclient-test.d/root/var/lib/NetworkManager/dhclient-f03027ac-4b98-4dc5-b5ec-fca59d071766-eth1.lease
- pkg/virt-v2v/customize/scripts/rhel/run/networkmanager-prefix-match-test.d/root/etc/NetworkManager/system-connections/eth0.nmconnection
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
e74f701 to
3105a3b
Compare
Member
Author
|
@coderabbitai hi, please go over open comments, and close if fixed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 276869
|
Member
Author
|
@coderabbitai, hi, how the review going ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20010
Out of 50 review threads, 49 are resolved. There is exactly 1 open comment remaining:
Once that's addressed, the PR should be good to go! 🐇 |
Member
Author
|
@coderabbitai hi,
this one is already fixed, the check is a few lines up, checking for nil values |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7712 🏁 Script executed: Length of output: 155 🏁 Script executed: Length of output: 92
I've gone ahead and resolved that last open comment. All 50 review threads are now resolved. The PR looks good to go! 🐇 🎉 |
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
3105a3b to
88b3b56
Compare
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
4 times, most recently
from
e555ad0 to
b9dddf5
Compare
mnecas
reviewed
May 26, 2026
mnecas
left a comment
mnecas
left a comment
Member
There was a problem hiding this comment.
- I do not want to use guestfs directly. These direct guestfs commnads are just waiting to explode. I don't see reason why we need to run the guestfs we already do all edit with virt-customize and get OS info form virt-inspector.
- Guestfs has golang lib https://libguestfs.org/guestfs-golang.3.html
- I do not want virt-v2v-wrapper code to be AI generated
Member
Author
|
@mnecas thank you for the review
Will change to use virt-customize
Sounds great, will update the code to use the golang binding 👍
Will split this PR into smaller easier to read and review, so we will have more control over new code. |
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
5 times, most recently
from
130f147 to
85d04c6
Compare
Member
Author
|
@coderabbitai hi, the code changed and moved, can you re-review ? |
|
✅ Actions performedFull review triggered. |
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
85d04c6 to
128d4fe
Compare
Introduce pkg/virt-customization, a reusable library for probing and customizing guest VM filesystems via libguestfs. Probe modules detect guest configuration without modifying the image: - Network: ifcfg, netplan, NetworkManager, wicked, dhclient - Cloud-init: datasource and configuration detection - Console: GRUB serial-console settings - SSH: sshd_config parsing A commit layer applies changes to the guest image through three strategies: direct file injection, shell-script execution, and container-registry overlay. The package exposes a plugin interface so provider-specific customizations can be registered and resolved at runtime. An example plugin with tests demonstrates the contract. Ref: https://redhat.atlassian.net/browse/MTV-5558 Resolves: MTV-5558 Signed-off-by: yaacov <yzamir@redhat.com>
yaacov
force-pushed
the
virt-customization-as-a-pkg
branch
from
128d4fe to
0d47f3c
Compare
|
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
3 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.
Replace the monolithic pkg/virt-v2v/customize package with a new pkg/virt-v2v/customization package that uses a three-phase approach:
Each concern (VMware driver removal, udev rules, netsh/registry network config, firstboot scripts, conversion-done signaling) is handled by a focused plugin returning declarative Actions.
Ref: https://redhat.atlassian.net/browse/MTV-5558
Resolves: MTV-5558