-
Notifications
You must be signed in to change notification settings - Fork 20
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
Decoupled API and frontend server, made code DRYer. Fixes #25 #26
Conversation
Signed-off-by: Soumya Deb <debloper@gmail.com>
@karnan4ever @kamleshverma1 please review, this is blocking TEN-90 |
@@ -129,6 +129,7 @@ class USMApp { | |||
$logProvider.debugEnabled(true); | |||
}]) | |||
.config(['RestangularProvider', function(RestangularProvider) { | |||
RestangularProvider.setBaseUrl('http://api.tendrl.org/1.0/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base URL cannot be hardcoded. also relative path should be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It will be config param in next up patches, as mentioned in Implement API endpoint as a config parameter for the frontend instance #27 (and in description) for the moment, it's closest to the same code it used to be (so that it doesn't break anything)
- It can't be relative path (in it's true form) unless the API and frontend are hosted in the same server & port, which is not the case.
this.rest = rest.withConfig((RestangularConfigurer) => { | ||
RestangularConfigurer.setBaseUrl('/api/v1/'); | ||
}); | ||
this.rest = rest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the use of this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it's set up so far, we're instantiating the resangular for every module - once for normal usage and one for more verbose usage (restAll
).
This is how we've lifted off the extra config, but I do realize the entire instantiating/rigging has to change eventually. IMO, we should do that once we have tests implemented (so that we don't introduce regressions).
This hasn't been implemented in most efficient way, but it works. So, I got rid of the non-DRY part (baseUrl
provided as default) and left the rest as is for now.
this.rest = rest.withConfig((RestangularConfigurer) => { | ||
RestangularConfigurer.setBaseUrl('/api/v1/gluster'); | ||
}); | ||
this.rest = rest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service is specific to gluster and not used anywhere. this file can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, maybe we should do a cleanups in next patches.
Obsoleted by #35 |
Signed-off-by: Soumya Deb debloper@gmail.com