refactor(api/machine): Introduce ParseNetwork for network string parsing#2724
refactor(api/machine): Introduce ParseNetwork for network string parsing#2724MahnoKropotkinvich wants to merge 1 commit intounikraft:stagingfrom
Conversation
MahnoKropotkinvich
requested review from
craciunoiuc and
nderjung
as code owners
There was a problem hiding this comment.
Pull request overview
Introduces a machine API–level ParseNetwork utility for parsing CLI network strings, reducing leakage of Unikraft-specific network parsing types into CLI code (per issue #1648).
Changes:
- Added
MachineNetwork+ParseNetwork/String()to standardize network string parsing/formatting in the machine API. - Refactored
kraft runnetwork parsing to usemachineapi.ParseNetworkinstead of ad-hoc parsing. - Updated
kraft compose createto emit network strings viamachineapi.MachineNetworkrather thanuknetdev.NetdevIp.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cli/kraft/run/utils.go | Switches network arg parsing to machineapi.ParseNetwork and maps parsed fields into the network interface spec. |
| internal/cli/kraft/compose/create/create.go | Removes uknetdev dependency; builds network strings using machineapi.MachineNetwork.String(). |
| api/machine/v1alpha1/network.go | Adds ParseNetwork and MachineNetwork.String() for colon-separated network string handling. |
| api/machine/v1alpha1/machine.zip.go | Introduces the MachineNetwork struct type in the machine API package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Handle CIDR default netmask if not provided. | ||
| if interfaceSpec.CIDR != "" { | ||
| ipMaskSplit := strings.SplitN(interfaceSpec.CIDR, "/", 2) | ||
| if len(ipMaskSplit) != 2 { | ||
| sz, _ := net.IPMask(net.ParseIP(found.Spec.Netmask).To4()).Size() | ||
| interfaceSpec.CIDR = fmt.Sprintf("%s/%d", interfaceSpec.CIDR, sz) | ||
| } | ||
| opts.IP = ipMaskSplit[0] | ||
| } |
There was a problem hiding this comment.
The --ip flag is currently ineffective: the value is never applied to interfaceSpec.CIDR when the network arg omits an IP, and opts.IP is overwritten from the parsed CIDR unconditionally. This makes --ip confusing and prevents it from doing what the flag description suggests. Consider using opts.IP to populate the interface CIDR when parsed.CIDR is empty, and avoid overwriting opts.IP when it was explicitly provided by the user.
There was a problem hiding this comment.
@craciunoiuc The --ip flag is never used either in both upstream branch or my PR. In this PR, I just introduced a new struct with its serialization function to replace the original uknetdev.NetdevIp. I kept almost the same population logic as the original code.
Since this PR is mainly focused on introducing a new struct for network string parsing, should I also include a bug fix for the --ip flag here, or keep it separate?
Signed-off-by: Mahno Kropotkinvich <mahno@libertarian.dev>
MahnoKropotkinvich
force-pushed
the
parse-network
branch
from
5e37963 to
946589e
Compare
Prerequisite checklist
Description of changes
Resolves #1648