feat(cve): add compare cves feature for 2 images#420
feat(cve): add compare cves feature for 2 images#420laurentiuNiculae wants to merge 1 commit intoproject-zot:mainfrom
Conversation
Signed-off-by: Laurentiu Niculae <niculae.laurentiu1@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
- Coverage 88.77% 81.73% -7.05%
==========================================
Files 56 58 +2
Lines 1720 1894 +174
Branches 463 508 +45
==========================================
+ Hits 1527 1548 +21
- Misses 182 333 +151
- Partials 11 13 +2 ☔ View full report in Codecov by Sentry. |
| allVulnerabilitiesForRepo: (name) => | ||
| `/v2/_zot/ext/search?query={CVEListForImage(image: "${name}"){Tag Page {TotalCount ItemCount} CVEList {Id Title Description Severity Reference PackageList {Name InstalledVersion FixedVersion}}}}`, | ||
| cveDiffForImages: (minuend = {}, subtrahend = {}, { pageNumber = 1, pageSize = 3 }) => { | ||
| let imageInput = (img) => { |
There was a problem hiding this comment.
if the value for the variable doesn't change, please use 'const' for declaration
| }, | ||
| allVulnerabilitiesForRepo: (name) => | ||
| `/v2/_zot/ext/search?query={CVEListForImage(image: "${name}"){Tag Page {TotalCount ItemCount} CVEList {Id Title Description Severity Reference PackageList {Name InstalledVersion FixedVersion}}}}`, | ||
| cveDiffForImages: (minuend = {}, subtrahend = {}, { pageNumber = 1, pageSize = 3 }) => { |
There was a problem hiding this comment.
'minuend' and 'subtrahend' don't sound very suggestive of what the parameter represents. Is there a better name we can use perhaps?
There was a problem hiding this comment.
I struggled finding a better name, minuend means "from which you subtract" and subtrahend "how much you subtract". These are the most concise terms I know but I agree that they're quite mathematical and unfriendly
There was a problem hiding this comment.
Add a comment in the JS code explaining these.
| allVulnerabilitiesForRepo: (name) => | ||
| `/v2/_zot/ext/search?query={CVEListForImage(image: "${name}"){Tag Page {TotalCount ItemCount} CVEList {Id Title Description Severity Reference PackageList {Name InstalledVersion FixedVersion}}}}`, | ||
| cveDiffForImages: (minuend = {}, subtrahend = {}, { pageNumber = 1, pageSize = 3 }) => { | ||
| let imageInput = (img) => { |
There was a problem hiding this comment.
maybe we will want to reuse this imageInput function in the future for other endpoints. We should declare it outside 'cveDiffForImages'
| let platform = img.platform ? `, platform: {os: ${img.platform.os}, arch: ${img.platform.arch}}` : ''; | ||
| return `{repo: "${img.repo}", tag: "${img.tag}"${digest}${platform}}`; | ||
| }; | ||
| let query = `/v2/_zot/ext/search?query={CVEDiffListForImages(minuend: ${imageInput( |
There was a problem hiding this comment.
no need for the 'query' variable, we can just return the interpolated string directly
| const paginationParam = `requestedPage: {limit:${pageSize} offset:${(pageNumber - 1) * pageSize} sortBy:RELEVANCE}`; | ||
| return `/v2/_zot/ext/search?query={GlobalSearch(${searchParam}, ${paginationParam}) {Images {RepoName Tag}}}`; | ||
| }, | ||
| imageSuggestionsWithPlatforms: ({ searchQuery = '""', pageNumber = 1, pageSize = 15 }) => { |
There was a problem hiding this comment.
Is there a point for this endpoint? Can we not achieve this already with the 'globalSearch'?
| const [platform, setPlatform] = useState(''); | ||
| const [suggestionData, setSuggestionData] = useState([]); | ||
| const [queryParams] = useSearchParams(); | ||
| const search = queryParams.get('search') || ''; |
There was a problem hiding this comment.
similar story, do we want the query string to control the value of this search component?
| } else { | ||
| setInputHelpText('Image Selected'); | ||
| setActivePlatformSelection(true); | ||
| let platforms = getImagePlatforms(event.selectedItem); |
There was a problem hiding this comment.
doesn't seem like we need this variable
| }; | ||
|
|
||
| const imageSearch = (value) => { | ||
| let tag = value.split(':')[1]; |
| if (suggestionResponse.data.data.GlobalSearch.Images) { | ||
| const suggestionParsedData = suggestionResponse.data.data.GlobalSearch.Images.map((el) => mapToImage(el)); | ||
| setSuggestionData(suggestionParsedData); | ||
| setActivePlatformSelection(false); |
There was a problem hiding this comment.
duplicate 'setActivePlatformSelection'
| @@ -1,5 +1,5 @@ | |||
| const hostConfig = { | |||
| auto: true, | |||
There was a problem hiding this comment.
we shouldn't include dev settings in the commit
3 participants
What type of PR is this?
Which issue does this PR fix:
Closes #2152
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this change require updates to the CNI daemonset config files to work?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.