feat: CRUD jobs configurations #831
Conversation
|
This PR will trigger a minor release when merged. |
martinst06
requested review from
solaris007 and
alinarublea
and removed request for
solaris007
|
dummy comment |
There was a problem hiding this comment.
The PR goes in a good direction. There are still some adjustments required as mentioned in the comments.
It would generally help reviewers to have a more visual way to check the rendered API docs alongside reviewing fragments of yamls, so we generally add in the PR description/comments screenshots of important sections changed.
docs/openapi/configurations-api.yaml
Outdated
| schema: | ||
| type: array | ||
| items: | ||
| $ref: './schemas.yaml#/Configuration' |
There was a problem hiding this comment.
Should it be just the job configuration? not the whole configuration including handlers, queues etc.
There was a problem hiding this comment.
While checking the Configuration schema, I realized handlers and slackRoles properties are not documented in our API docs. This could be a follow-up update to the docs.
docs/openapi/configurations-api.yaml
Outdated
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: './schemas.yaml#/Configuration' |
There was a problem hiding this comment.
Same as above, response should be just the jobs sections of the configuration, including the version alongside, since that may be helpful.
docs/openapi/configurations-api.yaml
Outdated
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: './schemas.yaml#/Configuration' |
| - configuration | ||
| summary: Delete the latest jobs configurations by type | ||
| description: | | ||
| This endpoint is useful for deleting the latest jobs configurations by a specific type. |
There was a problem hiding this comment.
Reading this, I'm unclear what delete means. Does it mean we create a new configuration version that doesn't contain that job type anymore? Could explain in the description?
| $ref: './responses.yaml#/401' | ||
| '500': | ||
| $ref: './responses.yaml#/500' | ||
| post: |
There was a problem hiding this comment.
The post needs a requestBody schema.
docs/openapi/schemas.yaml
Outdated
| JobType: | ||
| type: string | ||
| enum: | ||
| - 'import' |
There was a problem hiding this comment.
This is not a current job type, we should enumerate and give as example below existing job types such as: broken-backlinks, all-traffic, forms-opportunities etc.
Also, do we consider job type to be unique in the jobs list?
|
|
||
| const currentJobs = latestConfig.getJobs(); | ||
| const updatedConfig = await Configuration.update({ | ||
| jobs: [...currentJobs, ...jobsData], |
There was a problem hiding this comment.
This allows for duplicates to be pushed into the array, no? @solaris007, should we require job.type to be unique in the list and handle this in the validation/updated list accordingly?
| const updateData = context.body; | ||
|
|
||
| if (!isObject(updateData)) { | ||
| return badRequest('Update data must be an object'); |
There was a problem hiding this comment.
You should also validate the updateData or the final object obtained by the merge contains valid values for the required properties.
| version: latestConfig.getVersion(), | ||
| }); | ||
|
|
||
| return ok(ConfigurationDto.toJSON(updatedConfig)); |
There was a problem hiding this comment.
As mentioned above, if the response status code is 207, then the response should contain a status for each update. If partial success/failure is not a case here, then status response should rather be 200.
| $ref: './responses.yaml#/404' | ||
| '500': | ||
| $ref: './responses.yaml#/500' | ||
| patch: |
There was a problem hiding this comment.
The patch needs a requestBody schema.
…obe/spacecat-api-service into crud-schedules-audits-imports
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!