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 the ability to specify api via swagger in manifest #1078

Merged
merged 8 commits into from
Jan 10, 2020

Conversation

pwplusnick
Copy link
Contributor

Adds the ability for swagger file specifying the api to be
referenced in the manifest under project.config. The config
name was chosen to be consistent with whisk-cli -c/--config
option.

Note: I'll be adding a few documentation updates at the end of the day

Will Plusnick added 3 commits November 7, 2019 15:42
  Adds the ability for swagger file specifying the api to be
  referenced in the manifest under project.config. The config
  name was chosen to be consistent with whisk-cli -c/--config
  option.
  * Fix nil pointer error when no swagger present in undeploy
  * update godeps
@pwplusnick pwplusnick force-pushed the add_swagger_conf_to_manifest branch from 63eb7a4 to 3a39d3e Compare November 13, 2019 15:44
@mrutkows mrutkows added the review Issue is ready for review label Nov 13, 2019
@mrutkows
Copy link
Contributor

mrutkows commented Nov 13, 2019

@pwplusnick we should add as a follow-on PR the i18n error/warning messages/strings specific to swagger (config) document parsing/mgmt.

"Rev": "78658fd2d1772b755720ed8c44367d11ee5380d6"
},
{
"ImportPath": "github.com/ghodss/yaml",
Copy link
Member

Choose a reason for hiding this comment

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

Hey @pwplusnick, not sure how github.com/ghodss/yaml is different from gopkg.in/yaml.v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rewrite it, but it is used in a function I lifted from openwhisk-cli to do the swagger parsing. Ideally, we move that function to openwhisk-client-go and that dependency will get removed.

return err
var err error
// Only deploy either swagger or manifest defined api, but not both
// Swagger API takes precedence
Copy link
Member

Choose a reason for hiding this comment

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

please add caps NOTE saying the same, Swagger API take precedence over the apis defined in manifest YAML.

// Swagger API takes precedence
if deployer.Deployment.SwaggerApi != nil && deployer.Deployment.SwaggerApiOptions != nil {
err = deployer.createSwaggerApi(deployer.Deployment.SwaggerApi)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

missing check on err here

#
-->

# API Gateway
Copy link
Member

Choose a reason for hiding this comment

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

rename it to something more specific to API specifications?

- the root key is now project as the open api specification is a project wide concept.
- a new `config` key specifying where the Open API Specification is located.

The `config` key under `project` in the manifest file specifies where the Open API Specification is located. The keyword `config` was chosen to remain consistent with the OpenWhisk CLI flag option. The Open API Specification describes in a JSON document the the base path, endpoint, HTTP verb, and other details describing the API. For example, the document above describes a GET endpoint at `/hello/world` that recieves JSON as input and returns JSON as output.
Copy link
Member

Choose a reason for hiding this comment

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

@pritidesai
Copy link
Member

@pwplusnick are we checking if an action is a web action before creating an API using the specification? please confirm. wskdeploy checks if the action specified in manifest is a web action if not its created as web action if an api is requested for that action.

@pwplusnick
Copy link
Contributor Author

pwplusnick commented Nov 14, 2019

@pritidesai It does not check to see if the action is a web action. In fact, this does not do much checking on the swagger at all. It does a few very basic checks to make sure the base bath is valid, but apart from that it does not check the swagger on the client side. It basically shoves the swagger up to the api gateway and looks for errors there. It might be a good follow up PR to actually properly parse the swagger, and do that kind of checking.

Copy link
Member

@pritidesai pritidesai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @pwplusnick

@pritidesai pritidesai changed the title WIP: Add the ability to specify api via swagger in manifest Add the ability to specify api via swagger in manifest Jan 10, 2020
@pritidesai pritidesai merged commit a3a2bcb into apache:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Issue is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants