Merge pull request #309 from jonathannewman/PE-36370/main/remove-yaml…

…-support (PE-36370) remove yaml support
puppetlabs · Jun 26, 2023 · ae6807b · ae6807b
2 parents 383f621 + 64ab29f
commit ae6807b
Show file tree

Hide file tree

Showing 28 changed files with 373 additions and 296 deletions.
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
@@ -0,0 +1,24 @@
+{:linters {:unresolved-symbol {:level :warning :exclude [(puppetlabs.trapperkeeper.services/service)
+                                                         (puppetlabs.trapperkeeper.core/defservice)
+                                                         (puppetlabs.trapperkeeper.core/service)
+                                                         (puppetlabs.trapperkeeper.services/defservice)
+                                                         (clojure.test/is [thrown+? thrown+-with-msg? logged?])
+                                                         (puppetlabs.trapperkeeper.testutils.bootstrap/with-app-with-cli-data)
+                                                         (puppetlabs.trapperkeeper.testutils.bootstrap/with-app-with-config)
+                                                         (puppetlabs.trapperkeeper.testutils.bootstrap/with-app-with-empty-config)
+                                                         (puppetlabs.trapperkeeper.testutils.bootstrap/with-app-with-cli-args)
+                                                         (puppetlabs.trapperkeeper.testutils.logging/with-started)
+                                                         (puppetlabs.trapperkeeper.testutils.logging/with-logger-event-maps)
+                                                         (puppetlabs.trapperkeeper.testutils.logging/with-logged-event-maps)]}
+           :invalid-arity {:skip-args [puppetlabs.trapperkeeper.services/service
+                                       puppetlabs.trapperkeeper.services/defservice
+                                       puppetlabs.trapperkeeper.core/defservice
+                                       puppetlabs.trapperkeeper.core/service]}
+           :refer-all {:level :off}
+           :inline-def {:level :off}
+           :deprecated-var {:level :off}}
+
+ :lint-as {puppetlabs.trapperkeeper.core/defservice clojure.core/def
+           puppetlabs.trapperkeeper.services/defservice clojure.core/def
+           slingshot.slingshot/try+ clojure.core/try
+           puppetlabs.kitchensink.core/while-let clojure.core/let}}
diff --git a/.github/workflows/clojure-linting.yaml b/.github/workflows/clojure-linting.yaml
@@ -0,0 +1,30 @@
+name: Clojure Linting
+
+on:
+  pull_request:
+    types: [opened, reopened, edited, synchronize]
+    paths: ['src/**','test/**','.clj-kondo/config.edn','project.clj','.github/**']
+
+jobs:
+  clojure-linting:
+    name: Clojure Linting
+    runs-on: ubuntu-latest
+    steps:
+      - name: setup java
+        uses: actions/setup-java@v3
+        with:
+          distribution: temurin
+          java-version: 17
+      - name: checkout repo
+        uses: actions/checkout@v2
+      - name: install clj-kondo (this is quite fast)
+        run: |
+          curl -sLO https://raw.githubusercontent.com/clj-kondo/clj-kondo/master/script/install-clj-kondo
+          chmod +x install-clj-kondo
+          ./install-clj-kondo --dir .
+      - name: kondo lint
+        run: ./clj-kondo --lint src test
+      - name: eastwood lint
+        run: |
+          java -version
+          lein eastwood
diff --git a/.github/workflows/lein-test.yaml b/.github/workflows/lein-test.yaml
@@ -0,0 +1,29 @@
+name: PR Testing
+
+on:
+  workflow_dispatch:
+  pull_request:
+    types: [opened, reopened, edited, synchronize]
+    paths: ['src/**','test/**','project.clj']
+
+jobs:
+  pr-testing:
+    name: PR Testing
+    strategy:
+      fail-fast: false
+      matrix:
+        version: ['8', '11', '17']
+    runs-on: ubuntu-latest
+    steps:
+      - name: checkout repo
+        uses: actions/checkout@v3
+        with:
+          submodules: recursive
+      - name: setup java
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: ${{ matrix.version }}
+      - name: clojure tests
+        run: lein test
+        timeout-minutes: 30
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,10 @@
+## 4.0.0
+
+This is a major release with breaking changes.
+* remove support for yaml configuration files
+* add clj-kondo linting and fix issues found
+* add eastwood linting and fix issues found
+
 ## 3.3.1
 
 This is a maintenance release

diff --git a/dev-resources/config/file/config.yaml b/dev-resources/config/file/config.yaml
diff --git a/dev-resources/config/mixeddir/qux.yaml b/dev-resources/config/mixeddir/qux.yaml
diff --git a/project.clj b/project.clj
@@ -1,12 +1,12 @@
-(defproject puppetlabs/trapperkeeper "3.3.1-SNAPSHOT"
+(defproject puppetlabs/trapperkeeper "4.0.0-SNAPSHOT"
   :description "A framework for configuring, composing, and running Clojure services."
 
   :license {:name "Apache License, Version 2.0"
             :url "http://www.apache.org/licenses/LICENSE-2.0.html"}
 
   :min-lein-version "2.9.0"
 
-  :parent-project {:coords [puppetlabs/clj-parent "5.3.2"]
+  :parent-project {:coords [puppetlabs/clj-parent "6.0.1"]
                    :inherit [:managed-dependencies]}
 
   ;; Abort when version ranges or version conflicts are detected in
@@ -32,7 +32,6 @@
 
                  [clj-time]
                  [clj-commons/fs]
-                 [clj-commons/clj-yaml]
 
                  [prismatic/plumbing]
                  [prismatic/schema]
@@ -44,7 +43,8 @@
                  ;; see https://github.com/puppetlabs/trapperkeeper/pull/306#issuecomment-1467059264
                  [puppetlabs/kitchensink nil :exclusions [cheshire]]
                  [puppetlabs/i18n]
-                 [nrepl/nrepl]]
+                 [nrepl/nrepl]
+                 [io.github.clj-kondo/config-slingshot-slingshot "1.0.0"]]
 
   :deploy-repositories [["releases" {:url "https://clojars.org/repo"
                                      :username :env/clojars_jenkins_username
@@ -70,7 +70,25 @@
                        :classifiers ^:replace []}}
 
   :plugins [[lein-parent "0.3.7"]
-            [puppetlabs/i18n "0.8.0"]]
+            [jonase/eastwood "1.2.2" :exclusions [org.clojure/clojure]]
+            [puppetlabs/i18n "0.9.2"]]
+
+  :eastwood {:ignored-faults {:reflection {puppetlabs.trapperkeeper.logging [{:line 92}]
+                                           puppetlabs.trapperkeeper.internal [{:line 128}]
+                                           puppetlabs.trapperkeeper.testutils.logging true
+                                           puppetlabs.trapperkeeper.testutils.logging-test true
+                                           puppetlabs.trapperkeeper.services.nrepl.nrepl-service-test true
+                                           puppetlabs.trapperkeeper.plugins-test true}
+                              :local-shadows-var {puppetlabs.trapperkeeper.config-test true
+                                                  puppetlabs.trapperkeeper.services-test true
+                                                  java-service-example.java-service true
+                                                  puppetlabs.trapperkeeper.optional-deps-test true}
+                              :deprecations {puppetlabs.trapperkeeper.testutils.logging true
+                                             puppetlabs.trapperkeeper.testutils.logging-test true
+                                             puppetlabs.trapperkeeper.logging-test true}
+                              :def-in-def {puppetlabs.trapperkeeper.optional-deps-test true}}
+
+             :continue-on-exception true}
+
+  :main puppetlabs.trapperkeeper.main)
 
-  :main puppetlabs.trapperkeeper.main
-  )
diff --git a/src/puppetlabs/trapperkeeper/bootstrap.clj b/src/puppetlabs/trapperkeeper/bootstrap.clj
@@ -37,9 +37,9 @@
   a 2-item vector containing the namespace and the service name.  Throws
   an IllegalArgumentException if the line is not valid."
   [line :- schema/Str]
-  (if-let [[match namespace service-name] (re-matches
-                                           #"^([a-zA-Z0-9\.\-]+)/([a-zA-Z0-9\.\-]+)$"
-                                           line)]
+  (if-let [[_match namespace service-name] (re-matches
+                                            #"^([a-zA-Z0-9\.\-]+)/([a-zA-Z0-9\.\-]+)$"
+                                            line)]
     {:namespace namespace :service-name service-name}
     (throw+ {:type ::bootstrap-parse-error
              :message (i18n/trs "Invalid line in bootstrap config file:\n\n\t{0}\n\nAll lines must be of the form: ''<namespace>/<service-fn-name>''." line)})))
@@ -162,7 +162,7 @@
   [configs :- [schema/Str]]
   (for [config configs
         [line-number line-text] (indexed (map remove-comments (read-config config)))
-        :when (not (empty? line-text))]
+        :when (seq line-text)]
     {:bootstrap-file config
      :line-number (inc line-number)
      :entry line-text}))
@@ -186,12 +186,12 @@
    service->entry-map :- {(schema/protocol services/ServiceDefinition) AnnotatedBootstrapEntry}]
   (let [make-error-message (fn [service]
                              (let [entry (get service->entry-map service)]
-                               (i18n/trs "{0}:{1}\n{2}" (:bootstrap-file entry) (:line-number entry) (:entry entry))))]
-    (let [error-messages (for [[protocol-id services] duplicate-services]
-                           (i18n/trs "Duplicate implementations found for service protocol ''{0}'':\n{1}"
-                                     protocol-id
-                                     (string/join "\n" (map make-error-message services))))]
-      (IllegalArgumentException. (string/join "\n" error-messages)))))
+                               (i18n/trs "{0}:{1}\n{2}" (:bootstrap-file entry) (:line-number entry) (:entry entry))))
+        error-messages (for [[protocol-id services] duplicate-services]
+                         (i18n/trs "Duplicate implementations found for service protocol ''{0}'':\n{1}"
+                                   protocol-id
+                                   (string/join "\n" (map make-error-message services))))]
+      (IllegalArgumentException. (string/join "\n" error-messages))))
 
 (schema/defn check-duplicate-service-implementations!
   "Throws an exception if two services implement the same service protocol"
@@ -200,12 +200,12 @@
 
   ; Zip up the services and bootstrap entries and construct a map out of them
   ; to use as a lookup table below
-  (let [service->entry-map (zipmap services bootstrap-entries)]
-    ; Find duplicates base on the service id returned by calling service-def-id
-    ; on each service
-    (let [duplicates (find-duplicates services services/service-def-id)]
-      (when (not (empty? duplicates))
-        (throw (duplicate-protocol-error duplicates service->entry-map))))))
+  (let [service->entry-map (zipmap services bootstrap-entries)
+       ; Find duplicates base on the service id returned by calling service-def-id
+       ; on each service
+        duplicates (find-duplicates services services/service-def-id)]
+      (when (seq duplicates)
+        (throw (duplicate-protocol-error duplicates service->entry-map)))))
 
 (schema/defn ^:private resolve-service! :- (schema/protocol services/ServiceDefinition)
   "Given the namespace and name of a service, loads the namespace,
@@ -242,11 +242,11 @@
 
   Throws an IllegalArgumentException if there is a problem parsing the bootstrap
   entry, or if the service is found but it has an invalid service graph."
-  [{:keys [bootstrap-file line-number entry] :as bootstrap-entry} :- AnnotatedBootstrapEntry]
+  [{:keys [bootstrap-file line-number entry]} :- AnnotatedBootstrapEntry]
   (try+
     (let [{:keys [namespace service-name]} (parse-bootstrap-line! entry)]
       (resolve-service! namespace service-name))
-    (catch [:type ::missing-service] {:keys [message]}
+    (catch [:type ::missing-service] {:keys [_message]}
       (log/warn (i18n/trs "Unable to load service ''{0}'' from {1}:{2}" entry bootstrap-file line-number)))
     ; Catch and re-throw as java exception
     (catch [:type ::internal/invalid-service-graph] {:keys [message]}

diff --git a/src/puppetlabs/trapperkeeper/config.clj b/src/puppetlabs/trapperkeeper/config.clj
@@ -23,7 +23,6 @@
             [me.raynes.fs :as fs]
             [puppetlabs.kitchensink.core :as ks]
             [puppetlabs.config.typesafe :as typesafe]
-            [clj-yaml.core :as yaml]
             [puppetlabs.trapperkeeper.services :refer [service service-context]]
             [puppetlabs.trapperkeeper.logging :refer [configure-logging!]]
             [clojure.tools.logging :as log]
@@ -57,9 +56,6 @@
     #{".edn"}
     (edn/read (PushbackReader. (io/reader file)))
 
-    #{".yaml" ".yml"}
-    (yaml/parse-string (slurp file))
-
     (throw (IllegalArgumentException.
             (i18n/trs "Config file {0} must end in .conf or other recognized extension"
                       (-> file str pr-str))))))
@@ -68,7 +64,7 @@
   [config-data cli-data]
   (if-let [cli-restart-file (:restart-file cli-data)]
     (do
-      (if (get-in config-data [:global :restart-file])
+      (when (get-in config-data [:global :restart-file])
         (log/warnf (i18n/trs "restart-file setting specified both on command-line and in config file, using command-line value: ''{0}''"
                              cli-restart-file)))
       (assoc-in config-data [:global :restart-file] cli-restart-file))
@@ -89,7 +85,7 @@
     [path]
     (mapcat
      #(fs/glob (fs/file path %))
-     ["*.ini" "*.conf" "*.json" "*.properties" "*.edn" "*.yaml" "*.yml"])))
+     ["*.ini" "*.conf" "*.json" "*.properties" "*.edn"])))
 
 (defn load-config
   "Given a path to a configuration file or directory of configuration files,

diff --git a/src/puppetlabs/trapperkeeper/core.clj b/src/puppetlabs/trapperkeeper/core.clj
@@ -166,8 +166,9 @@
         (throw (ex-info (i18n/trs "Process exit requested")
                         (assoc (::exit result) :kind ::exit)))))))
 
-(defn- parse-args [args]
+(defn- parse-args
   "Returns valid CLIData or throws an ::exit."
+  [args]
   (letfn [(quit [status msg stream ex-msg]
             (throw (ex-info ex-msg
                             ;; :status and :messages match exit-request-schema
@@ -177,7 +178,7 @@
     (try
       (internal/parse-cli-args! (or args ()))
       (catch ExceptionInfo ex
-        (let [{:keys [kind msg] :as data} (ex-data ex)]
+        (let [{:keys [kind msg]} (ex-data ex)]
           (case (some-> kind without-ns)
             :cli-error (quit 1 msg *err* (i18n/trs "Invalid program arguments"))
             :cli-help (quit 0 msg *out* (i18n/trs "Command line --help requested"))

diff --git a/src/puppetlabs/trapperkeeper/internal.clj b/src/puppetlabs/trapperkeeper/internal.clj
@@ -58,10 +58,10 @@
           (let [dir-path (fs/parent restart-file-path)]
             (fs/mkdirs dir-path)
             (spit restart-file-path "1")))
-        (catch ArithmeticException e
+        (catch ArithmeticException _e
           (spit restart-file-path "1")
           (log/debug (i18n/trs "Number of restarts has exceeded Long/MAX_VALUE, resetting file to 1")))
-        (catch NumberFormatException e
+        (catch NumberFormatException _e
           (spit restart-file-path "1")
           (log/error (i18n/trs "Restart file is unparseable, resetting file to 1"))))))
 
@@ -71,7 +71,7 @@
   if the graph is invalid."
   [service-def]
   {:post [(satisfies? s/ServiceDefinition %)]}
-  (if-not (satisfies? s/ServiceDefinition service-def)
+  (when-not (satisfies? s/ServiceDefinition service-def)
     (throw+ {:type ::invalid-service-graph
              :message (i18n/trs "Invalid service definition; expected a service definition (created via `service` or `defservice`); found: {0}" (pr-str service-def))}))
   (if (service-graph? (s/service-map service-def))
@@ -108,7 +108,7 @@
   compilation / instantiation, and inspects it to see if the error data map
   represents a missing trapperkeeper service or function.  If so, throws a
   more meaningful exception.  If not, re-throws the original exception."
-  [e]
+  [^ExceptionInfo e]
   {:pre [(instance? ExceptionInfo e)]}
   (let [data (ex-data e)]
     (condp = (:error data)
@@ -194,7 +194,7 @@
   (let [;; call the lifecycle function on the service, and keep a reference
         ;; to the updated context map that it returns
         updated-ctxt  (lifecycle-fn s (get-in @app-context [:service-contexts service-id] {}))]
-    (if-not (map? updated-ctxt)
+    (when-not (map? updated-ctxt)
       (throw (IllegalStateException.
                (i18n/trs "Lifecycle function ''{0}'' for service ''{1}'' must return a context map (got: {2})"
                          lifecycle-fn-name
@@ -247,9 +247,9 @@
    shutdown-reason-promise :- IDeref]
   (log/debug (i18n/trs "Initializing lifecycle worker loop."))
   (async/go-loop []
-    (let [[task chan] (async/alts!
-                       [shutdown-channel lifecycle-channel]
-                       :priority true)]
+    (let [[task _chan] (async/alts!
+                         [shutdown-channel lifecycle-channel]
+                         :priority true)]
       (schema/validate LifeCycleTask task)
       (let [{:keys [type task-function]} task]
         (condp #(contains? %1 %2) type
@@ -518,7 +518,7 @@
   {:pre [(satisfies? a/TrapperkeeperApp app)]
    :post [(identical? app %)]}
   (when-let [shutdown-reason (get-app-shutdown-reason app)]
-    (if-let [shutdown-error (:error shutdown-reason)]
+    (when-let [shutdown-error (:error shutdown-reason)]
       (throw shutdown-error)))
   app)
 
@@ -605,9 +605,9 @@
     ;; finally, create the app instance
     (reify
       a/TrapperkeeperApp
-      (a/get-service [this protocol] (services-by-id (keyword protocol)))
-      (a/service-graph [this] graph-instance)
-      (a/app-context [this] app-context)
+      (a/get-service [_this protocol] (services-by-id (keyword protocol)))
+      (a/service-graph [_this] graph-instance)
+      (a/app-context [_this] app-context)
       (a/check-for-errors! [this] (throw-app-error-if-exists!
                                    this))
       (a/init [this]