Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

add --include-unused-deps #412

Merged
merged 3 commits into from
Oct 26, 2021
Merged

add --include-unused-deps #412

merged 3 commits into from
Oct 26, 2021

Conversation

skilly-lily
Copy link
Contributor

@skilly-lily skilly-lily commented Oct 25, 2021

Adds --include-unused-deps to the CLI options.

Fixes fossas/team-analysis#785

Dependencies can have zero or more environments out of the following:

  • Production
  • Test
  • Development
  • Other x, where x is some arbitrary text string.

A dependency is considered a "Used" when it satisfies 1 or more of the following criteria:

  • The dep has no environments.
  • The dep has a Production environment.
  • The dep has an Other x environment, for any value of x.

Without the new flag enabled, only Used dependencies are included in the final graph.
With the new flag enabled, none of the above logic is used, and all dependencies are included.

If/when we implement and enable server-side filtering, we will accept and ignore this option.

I tested using this Cargo.toml file (minimal trasitive deps).

[package]
name = "cargotest"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
anyhow = "1.0.44"

[dev-dependencies]
num_cpus = "1.13.0"

[build-dependencies]
fnv = "1.0.7"

@skilly-lily skilly-lily requested review from zlav and meghfossa October 25, 2021 22:51
Copy link
Contributor

@meghfossa meghfossa left a comment

Choose a reason for hiding this comment

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

LGTM.

From reading the code it seems straight forward (so did not repro locally)

@@ -1,5 +1,9 @@
# Spectrometer Changelog

## v2.18.2

- Adds support for `fossa analyze --include-unused-deps`, which prevents filtering out non-production dependencies. ([#412](https://github.com/fossas/spectrometer/pull/412))
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I want to NOT do that. I'd prefer if this wasn't easily discoverable. This is a stopgap between the current state of things and server-side retroactive filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we find that we would rather have it documented, then we can add it later, but I'm worried about people using it when they don't need it and then complaining. We get this question a lot, but usually, the answer we resolve to is "you never needed it in the first place". I'd rather keep that mode of operation than just default people to using this. Especially considering that they can't take it back, they instead have to re-analyze without the flag.

@skilly-lily skilly-lily merged commit 9630757 into master Oct 26, 2021
@skilly-lily skilly-lily deleted the all-deps branch October 26, 2021 19:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants