This repository has been archived by the owner on Apr 1, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jssblck
requested review from
a team and
meghfossa
and removed request for
a team
October 22, 2021 18:54
meghfossa
approved these changes
Oct 25, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better handled with - https://github.com/fossas/team-analysis/issues/722. If filter, are aptly applied at discovery, we shouldn't have to do these branching for optimization.
If this is time sensitive customer request, go ahead with the merge (and add comment on 722 - to remove this branching in future)
I did not test this locally, but changes seem small enough.
LGTM.
Changelog.md
Outdated
@@ -1,5 +1,9 @@ | |||
# Spectrometer Changelog | |||
|
|||
## v2.18.1 | |||
|
|||
- Special cases scans with a single VSI only filter to skip other analysis strategies ([$407](https://github.com/fossas/spectrometer/pull/407)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#407
instead of $407
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Today when we run filters on analysis targets, we do all the analysis and then filter after the fact. This works, generally, except in the face of very large projects- which are also the ones most commonly run with
--enable-vsi
. The failure mode in such very large projects is that analysis may take so long to complete that the user gives up, which also blocks our VSI use case (which can support projects of almost arbitrary size- we've successfully scanned projects around the size of AOSP, so millions of files and hundreds of gigabytes of data).Due to this, this PR adds special casing to the application to unblock VSI scans for very large projects. When the user runs with
--enable-vsi
and only--only-target vsi
, we skip running any other analysis on the project.If the user provides
only-paths
orexclude-paths
, we still work with that, but if the user provides multipleonly-target
options we fall back to standard filtering. So this special casing only kicks in when the user provides a singleonly-target
option, and that single option is literally "vsi".Acceptance criteria
Testing plan
I ran the following scans. Note that we're looking primarily to ensure that the "Running in VSI only mode" log lines only appear when we run with a single
only-target vsi
option.Risks
I don't think this is super risky.
References
Internal slack convo about a POC, DM me for details.
Checklist
docs/
.Changelog.md
if this change is externally facing. If this PR did not mark a release, I added my changes into an# Unreleased
section at the top.