Skip to content

[skip ci] Define an OpenAPI document for vic-machine#6132

Merged
zjs merged 1 commit intovmware:feature/vic-machine-servicefrom
zjs:feature/vic-machine-service-spec
Sep 14, 2017
Merged

[skip ci] Define an OpenAPI document for vic-machine#6132
zjs merged 1 commit intovmware:feature/vic-machine-servicefrom
zjs:feature/vic-machine-service-spec

Conversation

@zjs
Copy link
Member

@zjs zjs commented Aug 28, 2017

Use an OpenAPI (Swagger) document to define a preliminary API for
vic-machine based on the vic-machine API design document (#6116).

This document is expected to evolve as implementation proceeds.

Resolves #6015.

Note: this change builds on #6124 and, once approved, I would like this to land as the second commit on feature/vic-machine-service.

@zjs zjs requested a review from hickeng August 28, 2017 17:58
@zjs zjs force-pushed the feature/vic-machine-service-spec branch from 53f695e to 074099c Compare August 28, 2017 19:15
@zjs zjs requested review from jak-atx and jooskim August 28, 2017 20:18
"security": []
}
},
"/{target}": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be /targets/{target}? This would reduce the chances of target name colliding with other resource names under /.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because {target} must be a valid IP address or hostname, I think the risk of collision is low.

Given the current vic-machine operations, and general model that all VIC operations involve a vSphere resource of some sort, /version may be the only resource directly under /.

Copy link
Contributor

Choose a reason for hiding this comment

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

Name collisions aside, it just seems odd to address a target under /. We're not managing the target after all. As a consumer of the API, I would expect endpoints under / to be directly relevant to the vic-machine service itself, like /version.

"/{target}/datacenter/{datacenter}": {
"get": {
"summary": "Show information about the specified vSphere resources",
"description": "Making a `GET` request on a datacenter will return information about the state of the host firewall on those resources.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The path of the resource /{target}/datacenter/ does not indicate it has anything to do with firewalls. Can we change that reflect what the call is actually about?

Copy link
Member Author

Choose a reason for hiding this comment

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

I expect that the resource will include additional information; firewall status is just the first item.

We could expose separate resources for each piece of information (/{target}/datacenter/{datacenter}/firewall, /{target}/datacenter/{datacenter}/foo, /{target}/datacenter/{datacenter}/bar), but instead I think it makes sense to have a single /{target}/datacenter/{datacenter} endpoint with a response body like below, and add sub-resources on an as-needed basis if necessary:

{
    "firewall": { 
    },
    "foo": {
    },
    "bar": {
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is fine if more than just firewall info is going to be retrieved.

zjs added a commit to zjs/vic that referenced this pull request Aug 29, 2017
zjs added a commit to zjs/vic that referenced this pull request Aug 29, 2017
@zjs zjs self-assigned this Aug 29, 2017
// Make sure not to overwrite this file after you generated it because all your edits would be lost!

func main() {

Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was generated by the swagger tool.

Given that the file was generated, and go fmt didn't remove it, I'm inclined to leave it as-is.

If I start touching the file, I'm going to wonder if I should also remove line 55, wonder why there's no line between 32 and 33, but is a line between 50 and 52, and... none of that seems like a good use of time.

@zjs zjs changed the base branch from master to feature/vic-machine-service September 7, 2017 20:56
zjs added a commit to zjs/vic that referenced this pull request Sep 13, 2017
@zjs zjs force-pushed the feature/vic-machine-service-spec branch from 4e0f2a8 to ce3db37 Compare September 14, 2017 17:31
zjs added a commit to zjs/vic that referenced this pull request Sep 14, 2017
@zjs zjs force-pushed the feature/vic-machine-service-spec branch from ce3db37 to b17246d Compare September 14, 2017 20:57
zjs added a commit to zjs/vic that referenced this pull request Sep 14, 2017
@zjs zjs force-pushed the feature/vic-machine-service-spec branch from b17246d to a9e20a3 Compare September 14, 2017 21:01
@zjs zjs requested a review from jzt September 14, 2017 21:01
zjs added a commit to zjs/vic that referenced this pull request Sep 14, 2017
Use an OpenAPI (Swagger) document to define a preliminary API for
vic-machine based on the vic-machine API design document (vmware#6116).

This document is expected to evolve as implementation proceeds.
@zjs zjs force-pushed the feature/vic-machine-service-spec branch from a9e20a3 to 7def8c1 Compare September 14, 2017 21:15
@zjs zjs merged commit 1851535 into vmware:feature/vic-machine-service Sep 14, 2017
@zjs zjs deleted the feature/vic-machine-service-spec branch September 14, 2017 21:15
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.

5 participants