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

-main: accept an optional classpath argument #75

Merged
merged 5 commits into from
May 12, 2021
Merged

-main: accept an optional classpath argument #75

merged 5 commits into from
May 12, 2021

Conversation

vemv
Copy link
Collaborator

@vemv vemv commented May 11, 2021

By accepting a fixed user-provided string as the classpath, one can be sure that lein-nvd is not interfering in classpath computation and therefore one prevents false positives/negatives.

Fixes #46

Also gives a way to easily solve:

#50
#73
#74

@vemv
Copy link
Collaborator Author

vemv commented May 11, 2021

PR ready

tests are failing, but so is master (locally and also shown in 849d739)

I did verify that the proposed command works, in both its Lein and deps.edn variants

Copy link
Collaborator Author

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Amended with some refinements and an assert (as a lightweight test)

passed-classpath-string? (s/split classpath-string #":")
(.exists (io/file "deps.edn")) (clojure-cli-classpath)
:else (make-classpath))
json-str (json/write-str {"classpath" classpath})]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I simplified things a bit here by using json/write-str, this way the Lein and deps.edn helpers don't have to construct escaped strings by hand

Copy link
Owner

@rm-hull rm-hull left a comment

Choose a reason for hiding this comment

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

Looks fine 👍
I'll investigate the failure on master

@rm-hull
Copy link
Owner

rm-hull commented May 11, 2021

Since travis has gone all shit, this project really could do with moving to github actions

ooh, so it was already moved across 😊

@vemv
Copy link
Collaborator Author

vemv commented May 11, 2021

Alright!

Feel free to LMK when master is green again. Meanwhile I'll try to add a simple test (I think a bash script would be nice for exercising Lein + deps.edn + the new argument all in the sample place)

@rm-hull
Copy link
Owner

rm-hull commented May 11, 2021

Main branch is now green

By accepting a fixed user-provided string as the classpath, one can be sure that lein-nvd is not interfering in classpath computation and therefore one prevents false positives/negatives.

Fixes #46

Also gives a way to easily solve:

#50
#73
#74
@vemv
Copy link
Collaborator Author

vemv commented May 11, 2021

Looks like master went red again after f4a7187

@vemv
Copy link
Collaborator Author

vemv commented May 11, 2021

Progressing...

The fix that would also fix master is:

    (is (== 0 (get-in project [:nvd :highest-score])))

Not really sure it's the right fix though. Originally that test had the opposite intent. But updating dependencies can change things.

@vemv
Copy link
Collaborator Author

vemv commented May 12, 2021

Finally got the build green 😄 (with the caveat of #75 (comment))

Also, I used the occasion to introduce a JVM matrix in CI, so that we're sure lein-nvd runs correctly in all JVMs that Clojure officially supports (today: 8 and 11).

Similarly, the integration test script runs in a parallel job, for a faster feedback loop.

@@ -44,10 +44,10 @@
(update-db/-main "test/resources/self-test.json")
(let [project (check/-main "test/resources/self-test.json")]
(is (== 11.0 (get-in project [:nvd :fail-threshold])))
(is (== 5.0 (get-in project [:nvd :highest-score])))
(is (== 0 (get-in project [:nvd :highest-score])))
Copy link
Owner

Choose a reason for hiding this comment

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

slightly puzzled why this dropped from 7.5 to 5.0 and now to zero .. happy to take this for now, but think i will create an issue to update self-test.json so we have something that is > 0

@rm-hull rm-hull merged commit 4f21d3a into rm-hull:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lein-nvd's own dependencies shouldn't affect the analysis result
2 participants