clj-holmes / clj-watson

clojure deps SCA
Eclipse Public License 2.0
84 stars 9 forks source link

Locating `clj-watson.properties` on classpath seems a bit odd #70

Closed lread closed 3 months ago

lread commented 3 months ago

Currently

Clj-watson will automatically find clj-watson's config in clj-watson.properties if it is on the classpath.

It might be just me but..

I find this a bit odd, and although it is documented in the README:

either on the classpath you use to run clj-watson or via the -w / --clj-watson-properties command-line option

...it did not sink in for me on the first skim of the docs.

I do realize I can specify any properties file I like on the command line, ex:

clojure -M:clj-watson scan -p deps.edn -w ./clj-watson.properties

But I see clj-watson as a tool (maybe that's where I've gone wrong?). And in my experience, a tool typically automatically finds its config relative to a project root. When using clj-watson as a tool, I can't see myself configuring a :replace-paths in my deps.edn to locate it's config, I would instead reach for the -w cmd line arg.

Proposal

Not sure.

One idea is to automatically load ./.clj-watson/config.properties if found. And this is where folks could also plunk their false positive suppression config (see #55)?

But since clj-watson already searches the classpath for clj-watson.properties, we don't want to break things for folks who are using this behavior... so maybe just some rewording in the docs to focus more on the cmd line arg and less on the classpath search?

seancorfield commented 3 months ago

At work, we use :replace-paths ["development/watson"] to let Watson find the config there, but I agree it's a bit of a strange default (and we run it as if -M since nothing is on our classpath if :dev is omitted, since we're using Polylith).

If you run it with -T and just :extra-deps (or :replace-deps) in the alias which is sort of the default, then the classpath includes "." but then you have to use clj-watson.entrypoint/scan and deal with the key/value pair style of invocation (which all exists and works, including abbreviations, but is less ergonomic for string arguments I think).

lread commented 3 months ago

Related: the cli help is a bit confusing:

   -w, --clj-watson-properties S                                               [ONLY APPLIED IF USING DEPENDENCY-CHECK STRATEGY] Path of an additional, optional properties file.

Focusing in:

Path of an additional, optional properties file

lread commented 3 months ago

Oh... right, you were trying to tell me, now I get it. clj-watson.properties are merged with dependency-check.properties. Yeah, this all seems a bit weird.

Now I wonder why I need the option to provide --dependency-check-properties. It's all a little head-scratchy for a newcomer to clj-watson.

seancorfield commented 3 months ago

That option predates the clj-watson.properties stuff and it (currently) specifies an entire replacement of the base dependency-check.properties file which... is kind of an insane thing to want to do: you have to copy the file from the repo and then edit it, for things to work properly! Which is why I added clj-watson.properties in the first place.

So that means that if someone does specify --dependency-check-properties, it's going to be an entire file and Watson should only read that file and not try to read/merge the core dependencycheck.properties and the (Watson) dependency-check.properties default. Ugh!

lread commented 3 months ago

Ah! Makes more sense when I know the jagged history of it! :slightly_smiling_face:
Maybe we should consider deprecating --dependency-check-properties.

seancorfield commented 3 months ago

Yes, with all the other changes around properties and env vars, that will make sense.

seancorfield commented 3 months ago

Given the doc changes in PR #99 and the proposed additional doc changes in #66 is anything further actually needed now to address this issue?

lread commented 3 months ago

I still find it surprising that a tool would search the classpath for a config file, but we don't necessarily need to fix this weirdness. I think we've documented it well enough.

seancorfield commented 3 months ago

OK, then I'll close this out on the basis that we'll document the property cascade further in #66