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

Add Restful service #29

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Add Restful service #29

merged 1 commit into from
Apr 27, 2023

Conversation

baude
Copy link
Collaborator

@baude baude commented Apr 23, 2023

add restful service to VFKit which defaults to tcp://localhost:8081. The URL can be changed using the newly introduced --restful-uri option. The schemes may be tcp or unix (unix domain socket). The service for the unix domain implementation is incomplete and should be considered a TODO still. users can also opt out of the service with a --restful-uri none.

the restful service now replies to three endpoints:

/vm/state GET which returns the state of the vm
/vm/state POST which allows for a state change
/vm/inspect GET which returns basic information about the virtual machine

error handling for the endpoints needs to be completed still. these endpoints should return http response codes (like 200) based on the error handling. this can be completed in a subsequent PR.

/version GET should also probably be added. I was unsure if we should return the version of vfkit (my first inclination) or some sort of API version. maybe we should return both?

added tests for new content where applicable

Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Here is a first round of comments, I need to take a closer look at the REST API.

cmd/vfkit/root.go Outdated Show resolved Hide resolved
doc/usage.md Show resolved Hide resolved
doc/usage.md Show resolved Hide resolved
pkg/cmdline/cmdline.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/rest/config.go Outdated Show resolved Hide resolved
pkg/rest/rest.go Outdated Show resolved Hide resolved
pkg/config/state_change.go Outdated Show resolved Hide resolved
pkg/rest/define/config.go Show resolved Hide resolved
pkg/rest/define/config.go Outdated Show resolved Hide resolved
@cfergeau
Copy link
Collaborator

/version GET should also probably be added. I was unsure if we should return the version of vfkit (my first inclination) or some sort of API version. maybe we should return both?

yeah vfkit version is good to have (possibly with git hash if that's easy to get).
API version, if that's about the REST API, I'd keep this for later. I'm sure we'll want to bump the API some day, but will we do this through a /version entry point, through a /v2 in the URL, .. I have no idea.

@baude
Copy link
Collaborator Author

baude commented Apr 24, 2023

should be updated now

@baude baude force-pushed the rest branch 3 times, most recently from 8a21b0f to bdbc7d3 Compare April 25, 2023 19:16
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Overall looks good! A few comments up for discussion

pkg/cmdline/cmdline_test.go Outdated Show resolved Hide resolved
cmd.Flags().StringArrayVarP(&opts.Devices, "device", "d", []string{}, "devices")

cmd.Flags().StringVar(&opts.LogLevel, "log-level", "", "set log level")
cmd.Flags().StringVar(&opts.RestfulURI, "restful-uri", DefaultRestfulURI, "URI address for RestFul services")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for using none:// instead of "" as the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i prefer this approach because the default IS none and this saves us a conditional check later ...

cmd/vfkit/main.go Outdated Show resolved Hide resolved
pkg/rest/rest.go Outdated Show resolved Hide resolved
pkg/rest/rest.go Outdated Show resolved Hide resolved
pkg/rest/rest.go Outdated Show resolved Hide resolved
pkg/rest/rest.go Outdated Show resolved Hide resolved
@cfergeau
Copy link
Collaborator

/lgtm
/approve

add restful service to VFKit which defaults to `tcp://localhost:8081`.  The URL can be changed using the newly introduced --restful-uri
option.  The schemes may be `tcp` or `unix` (unix domain socket).  The service for the unix domain implementation is incomplete and
should be considered a TODO still.  users can also opt out of the service with a `--restful-uri none`.

the restful service now replies to three endpoints:

/vm/state GET which returns the state of the vm
/vm/state POST which allows for a state change
/vm/inspect GET which returns basic information about the virtual machine

error handling for the endpoints needs to be completed still.  these endpoints should return http response codes (like 200) based on
the error handling.  this can be completed in a subsequent PR.

/version GET should also probably be added. I was unsure if we should return the version of vfkit (my first inclination) or some
sort of API version.  maybe we should return both?

added tests for new content where applicable

Signed-off-by: Brent Baude <bbaude@redhat.com>
@baude
Copy link
Collaborator Author

baude commented Apr 26, 2023

I have updated the PR per your wishes

@cfergeau
Copy link
Collaborator

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Apr 27, 2023
@baude baude added the approved label Apr 27, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: baude, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@baude baude merged commit 6a9c788 into crc-org:main Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants