Skip to content

Conversation

@Hritik14
Copy link
Collaborator

Fixes #394
Current status
image

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 22, 2021

What are you fixing ?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 22, 2021

Regarding #394 , you should try clearing importer.last_run . The way GitDataSource work is that they will only fetch data which was made between current time and last run time. I'm guessing you're istio importer had some failure earlier but ended up updating the last_run.

@Hritik14
Copy link
Collaborator Author

I actually recreated the entire database to verify that it is not the case. Further, after inspecting I got that the updated_advisories method doesn't actually return anything at all as the self._updated_files were empty. All the files were in self._added_files

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 22, 2021

@Hritik14 I see, missed it. The changes package url calls and the re thing is making things worse. Reason beings

  1. The package url call should have order similar to it's string repr (type comes first).
  2. Upper casing a function variable is just misleading. Upper case means it's some global dataish thing

Just fix the issue by changing https://github.com/nexB/vulnerablecode/blob/3d66b4e82ee31422ab907d3388f739b768ffd2ac/vulnerabilities/importers/istio.py#L50 to files = self._added_files.union(self._updated_files)

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 22, 2021

Btw do squash these and rebase

@Hritik14
Copy link
Collaborator Author

Hritik14 commented Mar 22, 2021

to files = self._added_files.union(self._updated_files)

Are you sure we should union them both ? I'm asking because there exists two explicit functions
https://github.com/nexB/vulnerablecode/blob/c689e9d31bfe5dff8692e3b04503f1226660bb4d/vulnerabilities/data_source.py#L198 and https://github.com/nexB/vulnerablecode/blob/c689e9d31bfe5dff8692e3b04503f1226660bb4d/vulnerabilities/data_source.py#L205 to have them separated.

The comment in updated_advisories does mention to have everything in updated_advisories if the data source doesn't differentiate between added and updated files but the git system clearly does differentiate between them. It just so happens that istio repository doesn't have any updated files yet.

Also, I've made the brach even with main. I'd really like you to squash the commits via the PR method as you did earlier.

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 24, 2021

@Hritik14 re updated vs added union won't cause any problem, in the latter stages of the pipeline we recently started combining the yielded dataclasses. Need to create a ticket about this to update the DataSource api .

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 24, 2021

@Hritik14 re squashing . That works too. Ideally you want to sync your main branch with the one here and rebase your feature branch to your main branch. That would avoid the merge commit.

The problem with me squashing the commits is, loss in context. Git history should represent proper evolution of the codebase. I'll squash this PR, since the fix is trivial.

@Hritik14
Copy link
Collaborator Author

I had to force push in order to rebase. Please check.

@Hritik14
Copy link
Collaborator Author

re updated vs added union won't cause any problem, in the latter stages of the pipeline we recently started combining the yielded dataclasses. Need to create a ticket about this to update the DataSource api .

So should we completely remove the added and have only updated in the DataSource api ?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 25, 2021

So should we completely remove the added and have only updated in the DataSource api ?

Eventually yes, but that's not the priority now.

Hritik14 added a commit to Hritik14/vulnerablecode that referenced this pull request Mar 25, 2021
1. The package url call should have order similar to it's string repr (type comes first).
2. Upper casing a function variable is just misleading. Upper case means it's some global dataish thing (is_release)
3. Remove added_files function

Reference: aboutcode-org#395 (comment)

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@Hritik14
Copy link
Collaborator Author

@sbs2001 Done

As all the files in istio git repo are added and not updated, it is
mandatory to handle `self._added_files` properly which was ignored
earlier.
This fixes aboutcode-org#394

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
type was used where name had been and name where type should be.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
type was used where name had been and name where type should be.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
1. The package url call should have order similar to it's string repr (type comes first).
2. Upper casing a function variable is just misleading. Upper case means it's some global dataish thing (is_release)
3. Remove added_files function

Reference: aboutcode-org#395 (comment)

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@Hritik14
Copy link
Collaborator Author

@sbs2001 rebased

@sbs2001 sbs2001 merged commit 0b7dc3d into aboutcode-org:main Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No results in istio importer

2 participants