Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add fixed packages in vulnerabilities details in packages endpoint. #831

Merged
merged 1 commit into from
Aug 9, 2022
Merged

Add fixed packages in vulnerabilities details in packages endpoint. #831

merged 1 commit into from
Aug 9, 2022

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Aug 7, 2022

No description provided.

@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from 656708a to e20221d Compare August 7, 2022 20:19
@TG1999 TG1999 marked this pull request as draft August 8, 2022 07:31
@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from e20221d to a029347 Compare August 8, 2022 12:05
@TG1999 TG1999 requested a review from tdruez August 8, 2022 12:05
@TG1999 TG1999 marked this pull request as ready for review August 8, 2022 12:05
@TG1999 TG1999 changed the title Add Fixed package in affected by vulnerabilities in packages endpoint Add Fixed package in vulnerabilities in packages endpoint Aug 8, 2022
tdruez
tdruez requested changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

It looks like you have included changes completely unrelated to the goal of this PR:

  • Pagination
  • Ordering

vulnerabilities/api.py Outdated
serializer_class = PackageSerializer
paginate_by = 50
pagination.PageNumberPagination.page_size = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very wrong plus it's unrelated to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have cut a branch from #830, I will rebase this branch once #830 get's merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Not ideal to combine multiple un-merged and un-related branches for the code review process.

@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch 2 times, most recently from 9de782b to bca4ad0 Compare August 8, 2022 13:13
@tdruez tdruez self-requested a review August 9, 2022 05:18
tdruez
tdruez reviewed Aug 9, 2022
View reviewed changes
vulnerabilities/api.py Outdated
Comment on lines 136 to 137
fixed_packages = self.get_fixed_packages(package=package)
qs = package.vulnerabilities.filter(packagerelatedvulnerability__fix=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing, packagerelatedvulnerability__fix is used with True in get_fixed_packages then overridden with False here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This qs is to get those vulnerabilities on the package that affects this that' why packagerelatedvulnerability__fix is set to False, whereas get_fixed_packages sends all the packages that matches name, namespace, type, qualifiers of that package and also fixes the vulnerability that's why it is set to True

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that, the confusing part is your approach of reusing the same base qs and overriding an existing lookup.

@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from bca4ad0 to 1909039 Compare August 9, 2022 11:46
@TG1999 TG1999 changed the title Add Fixed package in vulnerabilities in packages endpoint Add Fixed packages in vulnerabilities details in packages endpoint Aug 9, 2022
@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from 1909039 to bf235bb Compare August 9, 2022 11:47
@TG1999 TG1999 changed the title Add Fixed packages in vulnerabilities details in packages endpoint Add fixed packages in vulnerabilities details in packages endpoint. Aug 9, 2022
@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from bf235bb to b3140dc Compare August 9, 2022 11:52
@TG1999
Move fixed_packages to vulnerabilities in packages endpoint aboutcode…
050d580 
…-org#809

Reference: aboutcode-org#809
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999 TG1999 force-pushed the fixed_package_in_affected_by_vulnerabilities branch from b3140dc to 050d580 Compare August 9, 2022 11:54
@TG1999 TG1999 merged commit 4791113 into aboutcode-org:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdruez tdruez tdruez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TG1999 @tdruez