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.

Sign up for GitHub

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

Jump to bottom

option to disable /models /routes routes #1135

Merged
merged 1 commit into from
Mar 3, 2015
Merged

option to disable /models /routes routes #1135

merged 1 commit into from
Mar 3, 2015

Conversation

greaterweb
Copy link
Contributor

PR for #1134

WIP though feedback ok for existing work.

Suggested Functionality

  • Enabled by default to preserve backwards compatibility.
  • Deprecated from the beginning, so that users are warned about this problem. Use depd for deprecation, that's what express uses too.

Also:

  • Tests to ensure /models /routes can be disabled

@greaterweb
Copy link
Contributor Author

Condensed into a single commit and added the functionality to limit the /models and /routes availability to just GET requests. @bajtos, this PR is ready for review whenever anyone has time.

bajtos
bajtos reviewed Feb 26, 2015
View reviewed changes
package.json
@@ -49,7 +49,8 @@
"stable": "^0.1.5",
"strong-remoting": "^2.13.2",
"uid2": "0.0.3",
"underscore.string": "~2.3.3"
"underscore.string": "~2.3.3",
"depd": "~1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Please use ^1.0.0 so that we are automatically picking up new compatible versions in the future.

@bajtos bajtos self-assigned this Feb 26, 2015
@bajtos
Copy link
Member

bajtos commented Feb 26, 2015

@greaterweb Good job, thank you for the pull request. Please address my comments above.

@greaterweb
Copy link
Contributor Author

Thank you for the feedback. I will correct these items and update the PR.

@greaterweb greaterweb force-pushed the fix/issue-1134 branch 2 times, most recently from 1c80abd to 57e8e6f Compare February 26, 2015 14:39
@greaterweb
Copy link
Contributor Author

Updates implemented as suggested by @bajtos, rewrote the history to keep it clean as one small commit.

I can't see why the Merged build is failing unfortunately. I'm getting prompted me to authorize StrongLoop Jenkins with permissions to read/write for all of my public and private repos, not sure I want to do that :-)

Should any additional updates be required, please feel free to let me know.

@bajtos
Copy link
Member

bajtos commented Feb 27, 2015

The code LGTM, thank you.

The tests are failing because depd does not work in the browser. My pull request #1140 will fix that, so we need to wait until it lands.

@bajtos
Copy link
Member

bajtos commented Mar 2, 2015

@greaterweb #1140 has been landed, could you please rebase your patch on top of the current master?

In case you haven't done this before, here is one of the many ways: git fetch upstream && git rebase -i upstream/master && git push -f (assuming that "upstream" remote points to strongloop/loopback).

@greaterweb
Copy link
Contributor Author

Sounds good, I'll get it ready.

@greaterweb greaterweb force-pushed the fix/issue-1134 branch 2 times, most recently from 2327df2 to 94b5b6c Compare March 2, 2015 19:11
@greaterweb
Config option to disable legacy explorer routes
1818a8f 
Setting legacyExplorer to false in the loopback config will disable
the routes /routes and /models made available in loopback.rest.
The deprecate module has been added to the project with a reference
added for the legacyExplorer option as it is no longer required by
loopback-explorer. Tests added to validate functionality of disabled
and enabled legacy explorer routes.
@greaterweb
Copy link
Contributor Author

Ok, should be good to go. Test are working ok locally but seem to be failing for CI.

Made a small update to remove my addition of depd from package.json and fixed the commit message as it mentioned limiting the supporting methods to just GET.

If you can see anything on my end that needs to be updated to fix the CI tests, please let me know.

@bajtos
Copy link
Member

bajtos commented Mar 3, 2015

For posterity, Travis CI is failing due to PhantomJS timeouts. Our Jenkins CI passed on 4 slaves out of 6, the remaining two slaves ran into race condition when checking out the code from git. It's a know problem of Jenkins which we did not managed to fix yet.

bajtos added a commit that referenced this pull request Mar 3, 2015
@bajtos
Merge pull request #1135 from greaterweb/fix/issue-1134
774c709 
ability to disalbe /models /routes routes
@bajtos bajtos merged commit 774c709 into strongloop:master Mar 3, 2015
@bajtos
Copy link
Member

bajtos commented Mar 3, 2015

Landed, thank you for the contribution!

@bajtos bajtos changed the title ability to disalbe /models /routes routes option to disable /models /routes routes Mar 3, 2015
@bajtos bajtos mentioned this pull request Mar 3, 2015
Merged
@kraman kraman mentioned this pull request Mar 4, 2015
Merged
@bajtos bajtos mentioned this pull request Apr 30, 2015
Closed
@davidcheung davidcheung mentioned this pull request Feb 4, 2016
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bajtos bajtos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@greaterweb @bajtos @altsang