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

address #103 Streamline dependency(-)check.properties overrides #106

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

seancorfield
Copy link
Contributor

Signed-off-by: Sean Corfield [email protected]

Signed-off-by: Sean Corfield <[email protected]>
@lread
Copy link
Contributor

lread commented Aug 19, 2024

Hi @seancorfield I think this PR supercedes #105? So I'll comment here.

Ideas for deprecating -d

(could be handled as a separate issue)

Option 1: Don't, continue to support.
Con: option is confusing
Con: we have to maintain and make sure it works

Option 2: Turf it, breaking change. This actually seems ok to me.
Con: breaks usage for x number of users. Maybe x = 0?
Pro: simplifies our life.

Option 3: Stop documenting the feature and warn if used. Usage and README would not describe feature and warn that it is slated for deletion if used.
Pro: gives users time to adapt.
Con: burden on us to test/support for a bit longer.

Our new Defaults

  1. For nvd db we are more CI-caching friendly, but there are might be considerations/tips we could add to README. Typically an .m2 CI cache is updated when certain files change, but our cache will now update with NVD db changes when no files have changed. I thought this was important for pomegranate and clj-yaml when caching with nvd-clojure, but we could create an issue to verify.

  2. What does nvd-clojure do? I've had a peek here. Differences:

They override where we don't:

  • central-enabled true (but this is default so OK)
  • experimental-enabled false (also default value)
  • jar-enabled true (also the default)
  • mix-audit-enabled false (OVERRIDES default)
  • nexus-enabled true (OVERRIDES default)
  • npm-cpe-enabled false (OVERRIDES default)
  • openssl-enabled false (OVERRIDES default)
  • ossindex-warn-only-on-remote-errors false (don't immediately see the default for this one, so probably false by default?)
  • yarn-audit-enabled false (OVERRRIDES default)

We override where they don't:

  • artifactory.enabled=false
  • carthage.enabled=false
  • dart.enabled=false
  • nexus.proxy=false
  • node.audit.use.cache=false
  • poetry.enabled=false
  • retirejs.filternonvulnerable=false

Override differences:

  • archive.enabled (us false them true)

Side thought: we'll want to diff on dependency-check.properties for new dependency check releases to see what has changed.

@lread
Copy link
Contributor

lread commented Aug 19, 2024

I'll see if I can rustle up some info on these differences.

@seancorfield
Copy link
Contributor Author

seancorfield commented Aug 19, 2024

Hi @seancorfield I think this PR supercedes #105? So I'll comment here.

Yes, thank you.

Re: deprecation -- I'm in favor of option 3, as long as the CLI lib has support for "hiding" options? We could suggest they migrate to using clj-watson.properties perhaps?

Re: caching -- I assume DC core's default data directory was selected so it would be CI-friendly with Maven caching, so I'm not too concerned about that. We can enhance the README later if folks raise questions about it.

Re: nvd-clojure differences -- I'm comfortable with our overrides that disable various analyzers since those were in clj-watson for quite some time (except a couple which seem like recent adds to DC core but should also be off for clj-watson). I'm curious why nvd-clojure has those additional defaults overridden, esp. disabling openssl? Thanks for following up on those.

@lread
Copy link
Contributor

lread commented Aug 19, 2024

nvd-clojure settings we don't have:

  • mix-audit-enabled false mix audit seems to be for Elixir. We could match nvd-clojure here.
  • nexus-enabled true nexus analyzer Superceded by central analyzer which is enabled by default. So we don't need to match nvd-clojure here.
  • npm-cpe-enabled false npm cpe - doesn't seem relevant to our usage, could match nvd-clojure here.
  • openssl-enabled false open ssl - seems like we could match nvd-clojure here.
  • ossindex-warn-only-on-remote-errors false oss index - is enabled by default... probably nvd-clojrue is setting this... So nvd-clojure maybe wants to fail, instead of warn? But this might be the default anyway. We can ignore for now I think.
  • yarn-audit-enabled false Seems not relevant to our use case, we can match nvd-clojure here.

And differences:

  • archive.enabled archive analyzer - we might want to match nvd-clojure here with a value of true

@lread
Copy link
Contributor

lread commented Aug 19, 2024

Re: deprecation -- I'm in favor of option 3, as long as the CLI lib has support for "hiding" options? We could suggest they migrate to using clj-watson.properties perhaps?

Yes, can do. I'll raise a separate issue.

Re: caching -- I assume DC core's default data directory was selected so it would be CI-friendly with Maven caching, so I'm not too concerned about that. We can enhance the README later if folks raise questions about it.

Somebody (hint, hint) is trying to tell you he might have real-world experience he might want to share that might benefit others. But can be slipped in with #84. So no need for an extra issue.

Signed-off-by: Sean Corfield <[email protected]>
@seancorfield
Copy link
Contributor Author

PR updated with analyzer property changes to align with nvd-clojure per your investigation -- thank you!

Somebody (hint, hint) is trying to tell you he might have real-world experience he might want to share that might benefit others.

[adopts very British accent] Good grief, man! Just spit it out! Tell the people what you want them to know...

(PR welcome 🙂 )

@seancorfield seancorfield merged commit fbdad5b into main Aug 19, 2024
5 checks passed
@seancorfield seancorfield deleted the issue-103-signed branch August 19, 2024 23:33
lread added a commit to lread/clj-watson that referenced this pull request Aug 20, 2024
…-line-arg

* upstream/main:
  address clj-holmes#103 Streamline dependency(-)check.properties overrides (clj-holmes#106)
  fixes clj-holmes#104 by supporting properties via environment variables (clj-holmes#108)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants