-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add rocky REST API for report recipes #3746
Conversation
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.
Looks good to me. Had just one tiny remark/ question
try: | ||
response = self._client.delete(f"/schedules/{schedule_id}") | ||
response.raise_for_status() | ||
except (HTTPStatusError, ConnectError): |
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.
A ConnectError
seems quite specific in this case. Perhaps a broader RequestError
would also suffice (or TransportError
to handle connection errors and timeouts)
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.
I actually just copypasted patch_schedule
and changed patch to delete without thinking about this. Most methods in the SchedulerClient
don't catch all the httpx exceptions. I think in most places we should just catch HTTPError
, but we should do this in a separate issue / PR.
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.
ideally we should not be dealing with the schedulerclient in both this and the other view. We should just be instantiating Schedule objects and calling on their crud methods.
The Schedule object itself would then abstract away the interaction with the SchedulerClient and catch any errors that are HTTP specific.
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 points. Maybe as an initial improvement to begin with, abstract away all the external dependency exceptions. For example, users that use a SchedulerClient
shouldn't necessarily know or care about httpx
and its exceptions, but they could expect a higher-level SchedulerError
(that could optionally wrap an httpx
exception for later reference) and its subclasses. And in the client's response hooks we could catch unhandled httpx
exceptions and reraise a proper SchedulerError
.
This could be an easy win for simplifying error management and improving maintainability and handle this situation since there are much less exceptions to handle, because we could just act on the generic SchedulerError
exception and render the appropriate message.
Obviously this applies to other clients with the codebase and external dependencies.
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.
Agreed. The clients should (as we discussed before) just wrap the common errors in an error class for easy consumption by the user. the user shouldnt need to know if the client talks to a database, an api or a filesystem.
Checklist for QA:
What works:QA steps successfully tested and working. Can also open the report for the deleted report recipe that is present in the History tab. What doesn't work:n/a Bug or feature?:n/a |
Quality Gate failedFailed conditions |
* main: Fix reports with organization tags (#3790) Update README.rst - Fix guidelines URLs (#3789) Exclude Report from ooi list (#3768) Update `croniter` (#3767) Fixes for dropdowns (#3732) Fix scheduled Aggregate Report naming (#3748) Make systemctl call for kat-rocky-worker conditional (#3782) Fix vulnerability chapters in Aggregate table of content (#3780) Fix auth token middleware with wrong format header (#3755) Add rocky REST API for report recipes (#3746) Add exception handling to the rest api (#3708) Refactor Multi Report to comply to the new report flow (#3705) Fix report names for scheduled reports (#3726) Fix Multi Report recursion error (#3714) Bump waitress from 3.0.0 to 3.0.1 in /octopoes (#3760) Docs/add muted findings (#3699) Edit report recipe (#3690) Add start date to report schedule (#3701) Add REST API to list report and download pdf report (#3689) Fixes in Report Overview (#3707)
Changes
This adds the rocky REST API to create report recipes.
Issue link
Closes #3526
QA notes
The following calls can be done to test the API using httpie:
List all recipe:
Get details of a single recipe:
Create new recipe:
Update recipe:
Delete recipe:
Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.