Closed BlackHole1 closed 6 months ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Hi @BlackHole1. Thanks for your PR.
I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
/ok-to-test
Test Code:
For what it's worth, if you want to add unit tests for this new code, it's now easier using https://github.com/crc-org/vfkit/blob/main/test/vm_test.go ,this makes it easy to create a VM with a set of devices, start vfkit, run some tests (eg make http request to the REST API), and check that the result is the expected one.
Test Code:
For what it's worth, if you want to add unit tests for this new code, it's now easier using https://github.com/crc-org/vfkit/blob/main/test/vm_test.go ,this makes it easy to create a VM with a set of devices, start vfkit, run some tests (eg make http request to the REST API), and check that the result is the expected one.
I've started doing this in https://github.com/cfergeau/vfkit/commit/4615740c4b96d99b7491dee8481246272c01108c but not finished yet :)
I've started doing this in cfergeau@4615740 but not finished yet :)
I'm now done with the testing code/..., I've filed https://github.com/crc-org/vfkit/pull/99 , in my opinion we can move the rest of the discussions in this new PR.
Hey @cfergeau, because it's Chinese New Year now, I'm currently on vacation and will be back in about a week.
Hey @cfergeau, because it's Chinese New Year now, I'm currently on vacation and will be back in about a week.
Thanks for the message, enjoy the holidays!
I'm back :)
I'm now done with the testing code/..., I've filed https://github.com/crc-org/vfkit/pull/99 , in my opinion we can move the rest of the discussions in this new PR.
Does it look like I can close this PR?
I'm back :)
I'm now done with the testing code/..., I've filed #99 , in my opinion we can move the rest of the discussions in this new PR.
Does it look like I can close this PR?
If the json format proposed in #99 for the inspect
API works for you, then yes we can close this PR.
If the json format proposed in https://github.com/crc-org/vfkit/pull/99 for the inspect API works for you
As for me, the new JSON format will make Go developers comfortable, but I cannot be certain about developers in other languages because they may not be able to determine the result of the response by inspecting the source code. Perhaps we still need to improve the relevant documentation?
I am also concerned that the new format may introduce more subtle breaking changes. For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API
, it could lead to breaking changes inadvertently.
Perhaps we still need to improve the relevant documentation?
Ah yes, for sure, I did not even think about documenting this yet :-/
For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API, it could lead to breaking changes inadvertently.
pkg/config/json_test.go
should catch many of these breaking changes, but it's not exhaustive, we could extend it to make sure it covers all the fields that are serialized to json. This way json breaking changes will be noticeable.
Perhaps we still need to improve the relevant documentation?
Ah yes, for sure, I did not even think about documenting this yet :-/
For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API, it could lead to breaking changes inadvertently.
pkg/config/json_test.go
should catch many of these breaking changes, but it's not exhaustive, we could extend it to make sure it covers all the fields that are serialized to json. This way json breaking changes will be noticeable.
I tried to do this in https://github.com/crc-org/vfkit/pull/103
Add the correct cpus, memory, devices
Test Code:
Result: