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.

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 support for reference_type #1518

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Jul 22, 2024

Reverts #1517

@TG1999 TG1999 mentioned this pull request Jul 22, 2024
@TG1999
Copy link
Contributor Author

TG1999 commented Jul 22, 2024

@ziadhany please see CI is faling here with Rust changes, please check this, so I can merge this and #1481 . Thanks!

@ziadhany
Copy link
Collaborator

@TG1999
Update the file vulnerabilities/tests/test_data/github_api/cargo-expected.json with the version from EPSS #1481.

@TG1999
Copy link
Contributor Author

TG1999 commented Jul 22, 2024

@ziadhany please can you push that here? You have access for same ?

@ziadhany
Copy link
Collaborator

@TG1999 I don't have the permission

remote: Permission to nexB/vulnerablecode.git denied to ziadhany.

this is a patch to fix the test

Fix_cargo_test.zip

@TG1999
Copy link
Contributor Author

TG1999 commented Jul 22, 2024

Try pushing again

Copy link
Contributor Author

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

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

LGTM!

TG1999 and others added 2 commits August 1, 2024 23:59
@ziadhany ziadhany force-pushed the revert-1517-revert-1502-ref-type branch from d9d6ba3 to 5aa2924 Compare August 1, 2024 21:12
@ziadhany
Copy link
Collaborator

ziadhany commented Aug 2, 2024

@TG1999 @johnmhoran I've encountered a strange issue in CI where the apache_kafka and apache_tomcat tests are failing intermittently. However, when I run the tests locally using the same Python version, they all pass. This might be due to the CI running tests in parallel. Do you have any suggestions on how we can resolve this?

@keshav-space
Copy link
Member

I've encountered a strange issue in CI where the apache_kafka and apache_tomcat tests are failing intermittently. However, when I run the tests locally using the same Python version, they all pass. This might be due to the CI running tests in parallel. Do you have any suggestions on how we can resolve this?

@ziadhany the issue is resolved, just needed to regenerate the apache kafka test fixture properly.

Also @ziadhany

  • Should we allow the reference_type to be empty, or should we use "UNKNOWN" when we cannot properly ascertain the reference type?
  • Additionally, what should we do with the already existing references? Should we assign them the reference type Advisory?
  • We will also need to update the existing importers to use the reference type. Do we have any follow-up issues for that?

@ziadhany
Copy link
Collaborator

ziadhany commented Aug 3, 2024

@keshav-space Thanks for solving this issue.

Should we allow the reference_type to be empty, or should we use "UNKNOWN" when we cannot properly ascertain the reference type?

I believe it makes more sense to allow reference_type to be empty.

Additionally, what should we do with the already existing references? Should we assign them the reference type Advisory?

I believe we shouldn't assign a reference type to them since not all references are Advisory. Instead, I think we should edit the process_inferences in improver_runner.py to merge the new references that have a reference_type.

I also think we should handle this in a separate pull request. This way, we can update any existing importer and write the proper test to ensure there are no ref duplications.

We will also need to update the existing importers to use the reference type. Do we have any follow-up issues for that?

Yes, I will create a few issues that we need to track to successfully update the reference type.

@TG1999
Copy link
Contributor Author

TG1999 commented Aug 5, 2024

@ziadhany is it good to be merged ?

@ziadhany
Copy link
Collaborator

ziadhany commented Aug 5, 2024

@TG1999 Yes, we can go ahead and merge.

@TG1999 TG1999 merged commit 84a35db into main Aug 6, 2024
11 checks passed
@keshav-space keshav-space deleted the revert-1517-revert-1502-ref-type branch September 18, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants