Skip to content
This repository has been archived by the owner on Jan 21, 2024. It is now read-only.

fixes #33 discovery mode #48

Closed
wants to merge 2 commits into from
Closed

fixes #33 discovery mode #48

wants to merge 2 commits into from

Conversation

brevity
Copy link
Contributor

@brevity brevity commented Mar 21, 2018

No description provided.

@coveralls
Copy link

coveralls commented Mar 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8566681 on 33_discovery_mode into f2da4ba on develop.

)
.demand(['f', 'p'])
.describe('f', 'Path to the RAML definition')
.describe('p', 'Port number to bind the proxy')
.describe('cors', 'Enable CORS with the API')
.describe('definition', 'URI of raml definition')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't required an extra parameter here. Instead, we should build the URL based on the API baseUri of the RAML file provided by -f.

@@ -48,6 +48,14 @@ function createServerFromBaseUri (raml, options) {
var app = osprey.Router()
var path = (raml.baseUri || '').replace(/^(\w+:)?\/\/[^/]+/, '') || '/'

if (options.definition) {
app.use('/resources', function (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why /resources? the way I read #33 is that it should work with all resources defined in the RAML definition.

@@ -48,6 +48,14 @@ function createServerFromBaseUri (raml, options) {
var app = osprey.Router()
var path = (raml.baseUri || '').replace(/^(\w+:)?\/\/[^/]+/, '') || '/'

if (options.definition) {
app.use('/resources', function (req, res) {
var body = '< link:"' +
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the < is extraneous. It appears in the example of #33 because that's how curl formats headers returned in responses.

Also, to be clear, this should be returned as a header, not in the body.

app.use('/resources', function (req, res) {
var body = '< link:"' +
options.definition +
'" rel="describedby" type="application/raml+yaml">'
Copy link
Contributor

Choose a reason for hiding this comment

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

The > here is wrong too. This should follow the header Link syntax as defined here.

@postatum postatum closed this Feb 27, 2020
@postatum postatum deleted the 33_discovery_mode branch February 27, 2020 08:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants