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

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

Closed
lread opened this issue Aug 1, 2024 · 9 comments
Closed

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

lread opened this issue Aug 1, 2024 · 9 comments
Assignees
Labels
needs analysis Further hammock time is required to figure out the best solution
Milestone

Comments

@lread
Copy link
Contributor

lread commented Aug 1, 2024

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
Copy link
Contributor

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).

@seancorfield seancorfield self-assigned this Aug 1, 2024
@seancorfield seancorfield added the needs analysis Further hammock time is required to figure out the best solution label Aug 1, 2024
@lread
Copy link
Contributor Author

lread commented Aug 1, 2024

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

  • The help implies that the properties file might be merged with something existing.
    Is that true? (have not checked code).
  • Also could state default behaviour of searching classpath in cli help.
  • Also could give an example usage (and/or be less ambiguous). The help might be taken to mean the dir of a clj-watson.properties file should be specified, I think it always wants the the path and filename.

@lread
Copy link
Contributor Author

lread commented Aug 1, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

lread commented Aug 2, 2024

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

@seancorfield
Copy link
Contributor

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

@seancorfield
Copy link
Contributor

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
Copy link
Contributor Author

lread commented Aug 16, 2024

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
Copy link
Contributor

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

@seancorfield seancorfield added this to the 6.0 milestone Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs analysis Further hammock time is required to figure out the best solution
Development

No branches or pull requests

2 participants