-
Notifications
You must be signed in to change notification settings - Fork 604
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
Show more VM details in the limactl list command #308
Conversation
pkg/store/instance.go
Outdated
Dir string `json:"dir"` | ||
Arch limayaml.Arch `json:"arch"` | ||
CPUs int `json:"cpus,omitempty"` | ||
Memory float64 `json:"memory,omitempty"` // bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the alternative would have been uint64
, but it doesn't fit into a JavaScript integer.
Basically using the same as the go-units library, so that the conversion goes smoother...
// BytesSize returns a human-readable size in bytes, kibibytes,
// mebibytes, gibibytes, or tebibytes (eg. "44kiB", "17MiB").
func BytesSize(size float64) string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure either of them actually work, so probably have to use string
or maps...
Can use int64 and do the conversion in the command instead, if it improves things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks fine to me. Maybe using string
would be better, so very large values can be round-tripped through JavaScript without losing precision, but don't really have strong feelings either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They used strings in podman, but then they also used uint64
for cpus (in case you have more than 2147483647 ?)
So while it is true that you can't have more than 9007199254740992 bytes, that's also a lot of memory/disk (8 PiB)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly should "hide" the docker dependency, in some local string/int64 conversion function ?
4c7d2d3
to
d4526a4
Compare
Add cpus / memory / disk, in addition to the arch. Use numbers for the machine-readable representation. (but use int64 instead of float64 in internal struct) Signed-off-by: Anders F Björklund <[email protected]>
9f4baee
to
c908af7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
We should detect the terminal width and limit the number of the columns correspondingly, but it can be worked out later.
Add cpus / memory / disk, in addition to the arch.
Use numbers for the machine-readable representation.