Add endpoint for getting list of components with their versions (#324) #335
Conversation
…components#324) 1. Changed the registry to anticipate 'Accept: application/vnd.oc.info+json' HTTP header and if detects it to skip the component rendering and to return the component's info only. 2. Added a new method in the Client - getComponentsInfo(...) It prepares the request to the registry with proper accept headers, checks for errors and provides a processed result pair (error[], info[]). - in case info data is provided for all the requested components then the error will be null and the info will have the following structure: [{ componentName: '<name>', requestedVersion: '<requested version|undefined>' apiResponse: { //// comes from the registry href: '<component url>', name: '<name>', renderMode: '<rendered|unrendered>', requestVersion: <requested version>, type: '<type>', version: '<actual version>' } }, { componentName: '<name>', requestedVersion: '<requested version|undefined>' apiResponse: {...} }, ... ] - in case of errors then the info part will skip the apiResponse and will include an error field: { componentName: '<name>', requestedVersion: '<requested version|undefined>' error: ... } In addition to this the 'error' will be an array holding the errors corresponding to each component requested. If there's no error for a particular component then the corresponding element in the 'error' array will be null: [ null, //// means the first component is OK { message: ..., stackTrace: ... }, null, //// third components is OK as well ... ]
There was a problem hiding this comment.
Hi @ivan-dimitrov-skyscanner
First, my apologies for the delay. I am on holiday now for Xmas but tried to do a quick review anyways 👍
Then, I added a couple of little comments. Overall it looks great, it's just some little improvements/questions.
Thanks for your help!
| @@ -1,6 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| var GetComponentHelper = require('./helpers/get-component'); | |||
| var Settings = require('../../resources/settings'); | |||
There was a problem hiding this comment.
Looks like this line is not actually needed?
| if (skipRendering) { | ||
| callback({ | ||
| status: 200, | ||
| response: response |
There was a problem hiding this comment.
This includes renderMode which is not actually very relevant here. Would you mind omitting that property in case skipRendering===true?
There was a problem hiding this comment.
Also, the problem of doing this here (instead of around line 78) is that here we go through validation and data closure execution, which I believe is not needed for the info purposes. I think we don't want to just skip rendering, but skip execution at all.
There was a problem hiding this comment.
Agree with you -- will move it around line 78 to skip most of the stuff done further in the file.
|
|
||
| it('should return a response with error code and description for the first component', function(){ | ||
| expect(response[0].response.code).to.be.equal('GENERIC_ERROR'); | ||
| expect(response[0].response.error).to.be.equal('Component execution error: thisDoesnotExist is not defined'); |
There was a problem hiding this comment.
As previous comment says, I actually am not sure we want this here. I think info should just skip execution at all and give us just the versions. What do you think?
|
|
||
| // it('should rerurn the actual version for the second component', function() { | ||
| // expect(response[1].response.version).to.be.equal('1.0.0'); | ||
| // }); |
There was a problem hiding this comment.
Can we get rid of those comments? I think now it is correctly resolving the versions which is what I would expect from info endpoints so those tests are not necessary anymore
There was a problem hiding this comment.
Ops, sorry about them. They weren't meant to be there.
Fixed!
- The actual info return was moved at the beginning of get-components.js so it will skip most of the validation and rendering logic when takes place - Fixed the tests so that they are not expecting href and renderMode anymore
i-b-dimitrov
force-pushed
the
324-Get-Components-Info
branch
from
0f6bb68
to
38dbbc2
Compare
|
Amazing, thanks! |
Changed the registry to anticipate 'Accept: application/vnd.oc.info+json'
HTTP header and if detects it to skip the component rendering and to return
the component's info only.
Added a new method in the Client - getComponentsInfo(...)
It prepares the request to the registry with proper accept headers, checks
for errors and provides a processed result pair (error[], info[]).
in case info data is provided for all the requested components then the error
will be null and the info will have the following structure:
[{
componentName: '',
requestedVersion: '<requested version|undefined>'
apiResponse: { //// comes from the registry
href: '',
name: '',
renderMode: '<rendered|unrendered>',
requestVersion: ,
type: '',
version: ''
}
},
{
componentName: '',
requestedVersion: '<requested version|undefined>'
apiResponse: {...}
},
...
]
in case of errors then the info part will skip the apiResponse and will
include an error field:
{
componentName: '',
requestedVersion: '<requested version|undefined>'
error: ...
}
In addition to this the 'error' will be an array holding the
errors corresponding to each component requested. If there's no
error for a particular component then the corresponding element in the 'error'
array will be null:
[
null, //// means the first component is OK
{ message: ..., stackTrace: ... },
null, //// third components is OK as well
...
]