Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make machine VMHost pointer #106

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

skatsaounis
Copy link
Collaborator

@skatsaounis skatsaounis commented Feb 3, 2025

  • Turn the VMHost machine field to a pointer to differentiate machines backed by a VM host (VMs) and physical machines.
  • Declare an internal MachineVMHost since only 3 fields of the machine's VM host are included in the response of the API link

Resolves: #27

SK1Y101
SK1Y101 previously approved these changes Feb 3, 2025
Copy link
Member

@SK1Y101 SK1Y101 left a comment

Choose a reason for hiding this comment

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

Looks good, but I wonder if the tests need changing, or are they fine?

@skatsaounis
Copy link
Collaborator Author

The tests are fine, but they are not capturing changes like this. What you can do to test it currently, is to use a dev MAAS and pick a real machine and a VM.

Tester go:

vm, err := client.Machine.Get("whhhdw")
if err != nil {
	fmt.Println(err)
}
fmt.Println(vm.VMHost.Name)

machine, err := client.Machine.Get("cwgwwh")
if err != nil {
	fmt.Println(err)
}
fmt.Println(machine.VMHost.Name)

Before the change:

$ go run main.go
1
0

After the change:

$ go run main.go
1
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x180 pc=0x6a7a54]

goroutine 1 [running]:
main.main()
	/home/skatsaounis/projects/go-playground/maasclient/main.go:264 +0x174
exit status 2

Which is fine since in order to fill it the machine should have a VM Host or pod

@skatsaounis skatsaounis requested a review from SK1Y101 February 3, 2025 14:52
Copy link
Member

@SK1Y101 SK1Y101 left a comment

Choose a reason for hiding this comment

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

Awesome!

@skatsaounis skatsaounis merged commit a7f2bb2 into canonical:master Feb 3, 2025
4 checks passed
@skatsaounis skatsaounis deleted the make-vmhost-optional branch February 3, 2025 15:24
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.

VMHOST/Pod in Machine should be a pointer instead of structure
3 participants