Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

public api #263

Merged
merged 4 commits into from
Aug 21, 2017
Merged

public api #263

merged 4 commits into from
Aug 21, 2017

Conversation

qibobo
Copy link
Contributor

@qibobo qibobo commented Aug 16, 2017

No description provided.

@cfdreddbot
Copy link

Hey qibobo!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link
Collaborator

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/150287500

The labels on this github issue will be updated when the story is started.

if (settings.publicPort < 1 || settings.publicPort > 65535) {
return {valid:false,message:"value of publicPort must between 1 and 65535"};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

how about add a check for public port != internal port ?

api/app.js Outdated
@@ -77,20 +101,35 @@ module.exports = function(configFilePath) {
});
}

var publicServer;
if(settings.publicTls){
publicServer = https.createServer(publicOptions, app).listen(publicPort || 3002, function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using different default port?

@qibobo
Copy link
Contributor Author

qibobo commented Aug 16, 2017

@cdlliuy fixed.

@cdlliuy
Copy link
Contributor

cdlliuy commented Aug 17, 2017

LGTM

return {valid:false,message:"publicPort must be a number"};
}
if (settings.publicPort < 1 || settings.publicPort > 65535) {
return {valid:false,message:"value of publicPort must between 1 and 65535"};
Copy link
Contributor

Choose a reason for hiding this comment

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

must -> must be

@boyang9527
Copy link
Contributor

this PR does not include the permission check with the oAuth token... I am assuming that one will be added later.

@qibobo
Copy link
Contributor Author

qibobo commented Aug 20, 2017

@boyang9527 fixed.

@qibobo
Copy link
Contributor Author

qibobo commented Aug 20, 2017

@boyang9527 will have another pr for oauth.

@boyang9527
Copy link
Contributor

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants