Skip to content

Add API endpoint to get revisions with CVEs data#5108

Merged
bartaz merged 4 commits intomainfrom
cve-get-revisions
Apr 23, 2025
Merged

Add API endpoint to get revisions with CVEs data#5108
bartaz merged 4 commits intomainfrom
cve-get-revisions

Conversation

@bartaz
Copy link
Copy Markdown
Member

@bartaz bartaz commented Apr 23, 2025

Done

  • renames previous /api/cve/[SNAP] endpoints to /api/[SNAP]/cves for consistency with other snap based endpoints
  • replaces previous endpoint that only checked for CVEs availability with an endpoint that returns list of revisions that have CVEs data

Current CVEs endpoints are:

  • /api/[SNAP]/cves - returns list of revisions that have CVEs data, or error if not found
  • /api/[SNAP]/[REVISION/cves - returns list of revisions for given revision of a snap

How to QA

Testing

  • This PR has tests
  • No testing required (explain why):

Issue / Card

Fixes WD-21559

@webteam-app
Copy link
Copy Markdown

@bartaz bartaz changed the title CVE get revisions Add API endpoint to get revisions with CVEs data Apr 23, 2025
@bartaz bartaz force-pushed the cve-get-revisions branch from b5dbf51 to e6e0ee2 Compare April 23, 2025 07:23
@bartaz bartaz marked this pull request as ready for review April 23, 2025 07:38
@bartaz bartaz requested a review from Copilot April 23, 2025 07:38
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the existing CVE API endpoints to improve consistency and functionality by renaming endpoints and updating the logic to return a list of revisions that have CVEs data rather than a simple boolean value.

  • Updated URL routes in views to follow a consistent naming pattern.
  • Renamed and refactored CVE API functions and helper methods to support returning revision numbers.
  • Updated test cases to reflect the new endpoint behavior and naming.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
webapp/publisher/snaps/views.py Updated API URL rules to match the new endpoint structure.
webapp/publisher/cve/cve_views.py Renamed function and updated logic to return revisions with CVEs.
webapp/publisher/cve/cve_helper.py Refactored helper to retrieve revision YAML files under the snap-cves folder.
tests/publisher/cve/test_has_cve_api.py Updated tests to use the new endpoint and mocked return values.
tests/publisher/cve/test_has_cve.py Renamed test class and updated tests for the new revisions retrieval.
Comments suppressed due to low confidence (1)

webapp/publisher/cve/cve_helper.py:114

  • [nitpick] The function name 'has_revisions_with_cves' implies a boolean return value, but it actually returns a list of revision numbers. Consider renaming it to 'get_revisions_with_cves' for clarity.
def has_revisions_with_cves(snap_name):

@steverydz
Copy link
Copy Markdown
Contributor

In the API response where there is an error, we usually use message instead of error, although I think error makes more sense, so perhaps we should continue with that going forwards?

Copy link
Copy Markdown
Contributor

@steverydz steverydz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Just one small comment and a question/statement

Comment thread webapp/publisher/cve/cve_views.py Outdated
revisions_with_cves = CveHelper.get_revisions_with_cves(snap_name)
if len(revisions_with_cves) > 0:
return flask.jsonify(
{"success": True, "revisions": revisions_with_cves}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other APIs we use data for the main data in the request, so in this case it would be "data": { "revisions": ... }

There are places we don't do that but they are older endpoints from before we started doing this.

@bartaz
Copy link
Copy Markdown
Member Author

bartaz commented Apr 23, 2025

In the API response where there is an error, we usually use message instead of error, although I think error makes more sense, so perhaps we should continue with that going forwards?

I guess consistency matters most at this point, so I'll update accordingly. But is is a potential space for future clean-up and maybe documenting it.

@bartaz bartaz requested a review from Copilot April 23, 2025 09:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new API endpoint to retrieve a list of snap revisions that include CVEs data and updates the URL patterns and supporting logic accordingly. Key changes include:

  • Renaming and updating API routes for CVE-related endpoints in the publisher views.
  • Refactoring the CVE helper function to return a list of revision numbers instead of a boolean.
  • Updating tests to match the new endpoint names and functionality.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
webapp/publisher/snaps/views.py Updated API routes to use the new endpoint naming convention.
webapp/publisher/cve/cve_views.py Renamed and refactored the view function to return revisions data.
webapp/publisher/cve/cve_helper.py Modified helper to fetch revisions with CVEs from file metadata.
tests/publisher/cve/test_has_cve_api.py Updated tests to assert the new revisions data returned by the endpoint.
tests/publisher/cve/test_has_cve.py Modified tests to reflect the change from boolean to list output.

@bartaz bartaz merged commit 3d3e831 into main Apr 23, 2025
11 checks passed
@bartaz bartaz deleted the cve-get-revisions branch April 23, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants