Skip to content

Add yaml decode function for machine info#25

Closed
jpm-canonical wants to merge 2 commits into
mainfrom
decode-yaml
Closed

Add yaml decode function for machine info#25
jpm-canonical wants to merge 2 commits into
mainfrom
decode-yaml

Conversation

@jpm-canonical

@jpm-canonical jpm-canonical commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Add YAML decoders for machine info and devices. This is required for backwards compatibility in the inference snap cli.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds YAML decoding support for MachineInfo and devices, while also renaming existing JSON decode entrypoints to make the format explicit.

Changes:

  • Renamed DecodeDecodeJSON across machine and device decode APIs.
  • Added YAML decode paths: machine.DecodeYAML, device.DecodeYAML, and per-bus DecodeYAML.
  • Updated/extended tests to cover the new decode functions (including YAML machine info decoding).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pkg/machine/machine_golden_test.go Updates golden test to use DecodeJSON.
pkg/machine/device/usb/usb.go Renames JSON decode function and adds YAML decode for USB devices.
pkg/machine/device/usb/usb_test.go Updates tests to use DecodeJSON.
pkg/machine/device/pci/pci.go Renames JSON decode function and adds YAML decode for PCI devices.
pkg/machine/device/pci/pci_test.go Updates tests to use DecodeJSON.
pkg/machine/device/fastrpc/fastrpc.go Renames JSON decode function and adds YAML decode for FastRPC devices.
pkg/machine/device/decode.go Introduces DecodeJSON and new DecodeYAML dispatch by bus.
pkg/machine/device/decode_test.go Updates tests to use DecodeJSON.
pkg/machine/decode.go Introduces DecodeJSON and new DecodeYAML for machine info.
pkg/machine/decode_test.go Updates JSON decode tests and adds YAML machine info decode tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/machine/decode.go
Comment thread pkg/machine/device/decode.go
Comment thread pkg/machine/device/usb/usb.go
Comment thread pkg/machine/device/pci/pci.go
Comment thread pkg/machine/device/fastrpc/fastrpc.go
Comment thread pkg/machine/device/decode.go

@farshidtz farshidtz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The changes are fine, but I'm not sure why this is needed in order to keep the inference snaps CLI backward compatible. That feels like a requirement that must be satisfied on the CLI module, although that is currently in beta and undergoing major changes anyway.

@jpm-canonical

Copy link
Copy Markdown
Contributor Author

Closing this as the team decided not to add YAML decoding support to lscompute at this stage. We are going to change modelctl debug select to only support JSON.

@jpm-canonical jpm-canonical deleted the decode-yaml branch June 30, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants