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

chore: Address little hack we added to quiet clj-holmes #81

Open
lread opened this issue Aug 3, 2024 · 1 comment
Open

chore: Address little hack we added to quiet clj-holmes #81

lread opened this issue Aug 3, 2024 · 1 comment

Comments

@lread
Copy link
Contributor

lread commented Aug 3, 2024

Background

While working on PR #80 checks initially failed clj-holmes checks (not clj-holmes/watson clj-holmes/clj-holmes!).

It turns out clj-holmes is a bit overzealous on what it considers a clojure spec.
It was convinced the map that describes our cli arguments was a spec, it saw :require true and assumed that we meant to type :required true.

Minimal example that triggers the clj-holmes finding:

(def foo {:bingo
          {:require true}})

The finding looks like this:

|                   :filename |               :name |                                                        :message | :severity | :lines |
|-----------------------------+---------------------+-----------------------------------------------------------------+-----------+--------|
| src/clj_watson/cli_spec.clj | Schema require typo | Typo on schema declaration using :require instead of :required. |     error |   [16] |

This would be very helpful if this we actually a clojure spec, but it is not.

We tried to reduce clj-holmes to only look at security related issues, and this worked locally via

clj-holmes scan -p . --rule-tags security

Alas there is to facility pass rule-tags to the clj-holmes github action we are using.

The Hack

There might be ways to suppress clj-holmes false positives but I didn't see a way after reading the docs. For now I've appeased clj-holmes by using :require :yes instead of :require true.

Options

Option 1: Do nothing

Leave the hack in.

Option 2: Wait for a fix from clj-holmes

  1. Ideally, this would be a fix for the false positive.
  2. Less ideally, a way to specify rule tags on the clj-holmes github action.

Option 3: Use clj-holmes without the github action

This would allow us to do 2.2 ourselves.

Option 4: Disable clj-holmes

I like that a security-related project is being scanned by a security tool, so I'd rather not.

Proposal

Let's wait a bit and see what happens with clj-holmes.
I'd rather option 2.1, but 2.2 or 3 would be ok as well.

@lread
Copy link
Contributor Author

lread commented Aug 4, 2024

Sean raised an issue over at clj-holmes: clj-holmes/clj-holmes-rules#41
And another for clj-holmes-action: clj-holmes/clj-holmes-action#10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant