Add Pulp API to UI #288
Add Pulp API to UI #288
Conversation
ZitaNemeckova
force-pushed
the
pulp_api
branch
2 times, most recently
from
71eae78 to
aca0047
Compare
Closes-Issue: AAH-255
ZitaNemeckova
force-pushed
the
pulp_api
branch
from
aca0047 to
f347655
Compare
|
@newswangerd please have a look, thanks :) |
| }); | ||
| } | ||
|
|
||
| list(params?: object, apiPath?: string) { |
There was a problem hiding this comment.
These are all copy pasted from BaseAPI. They should really be moved to a reusable file or inherited from a shared base class.
I know we were attempting to not change the base class for the full API, but I think it should be fairly easy to make a new base class that contains the list, get, update, create, delete, patch, getPath, authHandler and mapPageToOffset methods and the apiPath and http properties without introducing any regressions.
You could also just move the shared methods to an api.utils module and import them here and in base.ts. That might be a little bit easier, but I think it's not quite as clean. It's up to you.
| } | ||
| } | ||
|
|
||
| export const ExecutionEnvironmentAPI = new API(); |
There was a problem hiding this comment.
I like the idea of just putting all EE related calls into a single module rather than presenting the different objects in the pulp API to the presentation layer. I'm curious to see how this will work in the final implementation,
There was a problem hiding this comment.
One problem you might run into is that page loading will be blocked until all the APIs resolve. This is probably fine for details, but I know on the list views we'll have to make a call for each container to get the description, and that could slow down the list page a lot. If the APIs are called separately you could use a skeleton to show that the description is still loading: https://patternfly-react.surge.sh/components/skeleton. If all the descriptions are grabbed asynchronously, latency might not be an issue though.
src/api/pulp.ts
Outdated
| http: any; | ||
|
|
||
| constructor() { | ||
| this.apiPath = this.apiBaseURL; |
There was a problem hiding this comment.
I'm surprised these calls work without the CSRF token getting set. I guess it probably gets set by the other API calls and reused here. At any rate, it might be worth adding the authHandler here.
src/api/base.ts
Outdated
| apiPath: string; | ||
| http: any; | ||
|
|
||
| constructor() { | ||
| this.http = axios.create({ | ||
| baseURL: this.apiBaseURL, | ||
| baseURL: this.apiBaseURL(), |
There was a problem hiding this comment.
I would recommend passing this as a constructor argument instead of getting it from some function that you then have to override.
| // Use this function to get paths in the _ui API. That will ensure the API version | ||
| // gets updated when it changes | ||
| getUIPath(url: string) { | ||
| return `_ui/${this.UI_API_VERSION}/${url}`; |
There was a problem hiding this comment.
This function isn't necessary for anything other than the hub endpoints, so you can remove it here and just provide it in the hub base class.
src/api/pulp.ts
Outdated
| const params = { ...p }; | ||
| const sort = params['sort']; | ||
| params['ordering'] = sort; | ||
| delete params['sort']; |
There was a problem hiding this comment.
I would recommend moving mapPageToOffset to the base class and overriding the list method on the pulp class here to provide the mapping between ordering and sort.
Messing with sort in this function is kind of an unexpected side effect and this also makes it so that the actual code for the mapPageToOffset function is still duplicated in two places.
ZitaNemeckova
force-pushed
the
pulp_api
branch
from
542903d to
f50554c
Compare
Closes-Issue: AAH-255
Populate DB via console:
Last modified will be added in followup UI PR.
TODO: