Skip to content

Add task to prepopulate/pre-cache the NVD database#201

Open
sjamaan wants to merge 1 commit into
rm-hull:mainfrom
sjamaan:add-prepopulate-db-task
Open

Add task to prepopulate/pre-cache the NVD database#201
sjamaan wants to merge 1 commit into
rm-hull:mainfrom
sjamaan:add-prepopulate-db-task

Conversation

@sjamaan

@sjamaan sjamaan commented Jun 9, 2026

Copy link
Copy Markdown

This makes it easier to use in a CI pipeline, especially if multiple jobs use the NVD database.

Takes care of #162

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new prepopulate-db task to prepopulate the OWASP Dependency-Check database. The review feedback highlights a critical issue where unconditionally calling System/exit 0 inside the impl function halts the JVM, preventing programmatic usage (such as in a REPL or tests) and bypassing resource cleanup in the with-config block. The reviewer suggests implementing a conditional exit mechanism matching the pattern in the check task and fixing a minor typo ("Prepulating" to "Prepopulating").

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +50 to +61
(defn impl [config-filename]
(with-config [project config-filename]
(println "Prepulating database for" (-> project
:title
(s/trim)
(str "...")
(style :bright :yellow)))
(println " using nvd-clojure:" (:nvd-clojure @version) "and dependency-check:" (:dependency-check @version))
(-> project
do-updates)
;; If we got here, it's all good. Otherwise, we'd be throwing an exception
(System/exit 0)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Unconditionally calling System/exit 0 inside impl prevents programmatic usage of this task (e.g., from a REPL, tests, or other Clojure tools) and halts the JVM immediately. Furthermore, calling System/exit inside the with-config block prevents the finally block from executing, which means the engine's resources are not cleaned up properly.

We should introduce a conditional-exit helper that respects the :exit-after-check configuration option, matching the pattern used in nvd.task.check.

Additionally, there is a minor typo in the log message: Prepulating should be Prepopulating.

(defn conditional-exit [{:keys [exit-after-check] :as project}]
  (if exit-after-check
    (System/exit 0)
    project))

(defn impl [config-filename]
  (with-config [project config-filename]
    (println "Prepopulating database for" (-> project
                                             :title
                                             (s/trim)
                                             (str "...")
                                             (style :bright :yellow)))
    (println "  using nvd-clojure:" (:nvd-clojure @version) "and dependency-check:" (:dependency-check @version))
    (-> project
        do-updates
        conditional-exit)))

This makes it easier to use in a CI pipeline, especially if multiple
jobs use the NVD database.

Takes care of rm-hull#162
@sjamaan sjamaan force-pushed the add-prepopulate-db-task branch from 5fda905 to 76fbc9e Compare June 9, 2026 09:47
@sjamaan

sjamaan commented Jun 9, 2026

Copy link
Copy Markdown
Author

The bot has a point. The exit-after-check key does not get honored by this task, because the task isn't a "check" task. I'm not sure what to do here. There currently aren't really any tests of the tasks, anyway, so maybe it's a moot point?

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.

1 participant