-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat(rest): add can* params in GET /vm/state
#63
Conversation
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
One minor comment, such an API will be racy, the VM state can change between the I'm not sure
Or when changing state, we could use different errors for "the operation is impossible in this state" and "there was an error when performing the operation"? |
Good catch! I have changed the code. |
GET /vm/state
@cfergeau Done |
In the previous API, there was no way to detect before changing the status, which would make it difficult for developers to handle the following situations: - When the virtual machine status is Stop, calling Pause may fail. - When the virtual machine status is Pause, setting the status to Pause again may fail. In the previous solution, developers needed to first get the current status of the virtual machine and then decide whether to proceed to the next step. However, the current virtual machine status is very diverse (see: https://github.com/Code-Hex/vz/blob/bd29a7ea3d39465c4224bfb01e990e8c220a8449/virtualization.go#L23), which would require developers to handle various cases perfectly. This PR solves this problem.
I've squashed all commits to a single one, and copied the PR description to the commit, and force-pushed to your |
/lgtm (I'll approve after CI runs) |
NP. In the future, I will pay attention to this when I submit a PR. |
The expectations vary between projects, I don't mind doing the adjustments. I did it myself to avoid additional back & forth |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In the previous API, there was no way to detect before changing the status, which would make it difficult for developers to handle the following situations:
In the previous solution, developers needed to first get the current status of the virtual machine and then decide whether to proceed to the next step. However, the current virtual machine status is very diverse (see: https://github.com/Code-Hex/vz/blob/bd29a7ea3d39465c4224bfb01e990e8c220a8449/virtualization.go#L23), which would require developers to handle various cases perfectly. This PR solves this problem.