Skip to content

Commit

Permalink
Refactor reading of state to unify different ways of loading data and…
Browse files Browse the repository at this point in the history 
… be more robust against paths with invalid build numbers (related to #192)
  • Loading branch information
@flosell
flosell committed Sep 22, 2018
1 parent b6f08a1 commit b068d1f
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 44 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -4,6 +4,12 @@ This changelog contains a loose collection of changes in every release. I will a

The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to a "shifted" version of semantic versioning while the major version remains at 0: Minor version changes indicate breaking changes, patch version changes should not contain breaking changes.

## 0.14.3

### Fixed

* Made loading pipeline history a bit more robust in an attempt to improve the fix for #192

## 0.14.2

### Fixed
Expand Down
6 changes: 3 additions & 3 deletions src/clj/lambdacd/internal/default_pipeline_state.clj
View file
Expand Up @@ -9,7 +9,7 @@
(def clean-pipeline-state {})

(defn initial-pipeline-state [{home-dir :home-dir}]
(persistence/read-build-history-from home-dir))
(persistence/read-build-state-from home-dir))

(defn- update-step-result [new-step-result current-step-result]
(let [now (t/now)]
Expand Down Expand Up @@ -73,8 +73,8 @@
(defn new-default-pipeline-state [config & {:keys [initial-state-for-testing]}]
(let [home-dir (:home-dir config)
state-atom (atom (or initial-state-for-testing (initial-pipeline-state config)))
structure-atom (atom (persistence/read-build-datas home-dir "pipeline-structure.edn"))
build-metadata-atom (atom (persistence/read-build-datas home-dir "build-metadata.edn"))
structure-atom (atom (persistence/read-normal-build-data-from home-dir "pipeline-structure.edn"))
build-metadata-atom (atom (persistence/read-normal-build-data-from home-dir "build-metadata.edn"))
max-builds (or (:max-builds config) Integer/MAX_VALUE)
instance (->DefaultPipelineState state-atom structure-atom build-metadata-atom home-dir max-builds)]
instance))
61 changes: 31 additions & 30 deletions src/clj/lambdacd/internal/default_pipeline_state_persistence.clj
View file
Expand Up @@ -9,7 +9,8 @@
[clj-time.coerce :as c]
[clojure.edn :as edn]
[clojure.walk :as walk]
[me.raynes.fs :as fs]))
[me.raynes.fs :as fs]
[clojure.tools.logging :as log]))

(defn convert-if-instance [c f]
(fn [x]
Expand Down Expand Up @@ -44,18 +45,9 @@
(defn find-build-number-in-path [path]
(second (re-find #"build-(\d+)" (str path))))

(defn- build-number-from-path [path]
(sugar/parse-int (find-build-number-in-path path)))

(defn- build-state-path [dir]
(io/file dir "build-state.edn"))

(defn- read-build-edn [path]
(let [build-number (build-number-from-path path)
data-str (slurp path)
state (formatted-step-ids->pipeline-state (dates->clj-times (edn/read-string data-str)))]
{build-number state}))

(defn- write-build-edn [path build]
(let [serializable-build (clj-times->dates (pipeline-state->formatted-step-ids build))
state-as-edn-string (pr-str serializable-build)]
Expand All @@ -64,9 +56,8 @@
(defn- build-dirs [home-dir]
(let [dir (io/file home-dir)
home-contents (file-seq dir)
directories-in-home (filter #(.isDirectory %) home-contents)
build-dirs (filter find-build-number-in-path directories-in-home)]
build-dirs))
directories-in-home (filter #(.isDirectory %) home-contents)]
directories-in-home))

(defn- build-dir [home-dir build-number]
(let [result (str home-dir "/" "build-" build-number)]
Expand All @@ -83,30 +74,40 @@
(defn file-exists? [f]
(.exists f))

(defn read-build-history-from [home-dir]
(->> (build-dirs home-dir)
(map build-state-path)
(filter file-exists?)
(map read-build-edn)
(into {})))


(defn write-build-data-edn [home-dir build-number pipeline-data filename]
(let [f (io/file (build-dir home-dir build-number) filename)]
(spit f (pr-str pipeline-data))))

(defn- read-pipeline-structure-edn [f]
(let [build-number (build-number-from-path f)]
(if (file-exists? f)
{build-number (edn/read-string (slurp f))}
{build-number :fallback})))
(defn- read-edn-file [path]
(if (file-exists? path)
(edn/read-string (slurp path))))

(defn- build-files [home-dir filename]
(map #(io/file % filename) (build-dirs home-dir)))

(defn read-build-datas [home-dir filename]
(->> (build-dirs home-dir)
(map #(io/file % filename))
(map read-pipeline-structure-edn)
(defn- process-build-data-file [f post-processor]
(if-let [build-number-str (find-build-number-in-path f)]
(post-processor (read-edn-file f) (sugar/parse-int build-number-str))
(log/debug f "doesn't seem to contain a valid build number, skipping")))

(defn- read-and-process-data-files [home-dir filename post-processor]
(->> (build-files home-dir filename)
(map #(process-build-data-file % post-processor))
(into {})))

(defn- post-process-build-state-edn [data build-number]
(if data
{build-number (formatted-step-ids->pipeline-state (dates->clj-times data))}))

(defn- post-process-pipeline-structure-edn [data build-number]
{build-number (or data :fallback)})

(defn read-normal-build-data-from [home-dir filename]
(read-and-process-data-files home-dir filename post-process-pipeline-structure-edn))

(defn read-build-state-from [home-dir]
(read-and-process-data-files home-dir "build-state.edn" post-process-build-state-edn))

(defn clean-up-old-builds [home-dir old-build-numbers]
(doall (map (fn [old-build-number]
(fs/delete-dir (build-dir home-dir old-build-number))) old-build-numbers)))
22 changes: 11 additions & 11 deletions test/clj/lambdacd/internal/default_pipeline_state_persistence_test.clj
View file
Expand Up @@ -14,25 +14,25 @@
'(0 1 2) {:status :failure :out "something went wrong"}}}
home-dir (temp-util/create-temp-dir)]
(write-build-history home-dir 3 some-pipeline-state)
(is (= some-pipeline-state (read-build-history-from home-dir)))))
(is (= some-pipeline-state (read-build-state-from home-dir)))))
(testing "that string-keys in a step result are supported as well (#101)"
(let [some-pipeline-state {3 {'(0) {:status :success :_git-last-seen-revisions {"refs/heads/master" "some-sha"}}}}
home-dir (temp-util/create-temp-dir)]
(write-build-history home-dir 3 some-pipeline-state)
(is (= some-pipeline-state (read-build-history-from home-dir)))))
(is (= some-pipeline-state (read-build-state-from home-dir)))))
(testing "that keyworded values in a step result are suppored as well (#101)"
(let [some-pipeline-state {3 {'(0) {:status :success :v :x}}}
home-dir (temp-util/create-temp-dir)]
(write-build-history home-dir 3 some-pipeline-state)
(is (= some-pipeline-state (read-build-history-from home-dir))))))
(is (= some-pipeline-state (read-build-state-from home-dir))))))
(testing "generic build-data"
(testing "the standard case"
(let [home-dir (temp-util/create-temp-dir)
some-pipeline-structure pipeline-structure-test/foo-pipeline-display-representation]
(write-build-data-edn home-dir 1 some-pipeline-structure "pipeline-structure.edn")
(write-build-data-edn home-dir 2 some-pipeline-structure "pipeline-structure.edn")
(is (= some-pipeline-structure (get (read-build-datas home-dir "pipeline-structure.edn") 1)))
(is (= some-pipeline-structure (get (read-build-datas home-dir "pipeline-structure.edn") 2)))))))
(is (= some-pipeline-structure (get (read-normal-build-data-from home-dir "pipeline-structure.edn") 1)))
(is (= some-pipeline-structure (get (read-normal-build-data-from home-dir "pipeline-structure.edn") 2)))))))

(deftest clean-up-old-builds-test
(testing "cleaning up old history"
Expand Down Expand Up @@ -64,28 +64,28 @@
(deftest read-build-history-from-test ; covers only edge-cases that aren't coverd by roundtrip
(testing "that it will return an empty history if no state has been written yet"
(let [home-dir (temp-util/create-temp-dir)]
(is (= {} (read-build-history-from home-dir)))))
(is (= {} (read-build-state-from home-dir)))))
(testing "that it ignores build directories with no build state (e.g. because only structure has been written yet"
(let [home-dir (temp-util/create-temp-dir)]
(.mkdirs (io/file home-dir "build-1"))
(is (= {} (read-build-history-from home-dir)))))
(is (= {} (read-build-state-from home-dir)))))
(testing "that it ignores build that don't have a valid build number"
(let [home-dir (temp-util/create-temp-dir)]
(write-build-history home-dir "not-a-build-number" {})
(is (= {} (read-build-history-from home-dir))))))
(is (= {} (read-build-state-from home-dir))))))

(deftest read-pipeline-datas-test ; covers only edge-cases that aren't covered by roundtrip
(testing "that it will return an empty data if no state has been written yet"
(let [home-dir (temp-util/create-temp-dir)]
(is (= {} (read-build-datas home-dir "pipeline-structure.edn")))))
(is (= {} (read-normal-build-data-from home-dir "pipeline-structure.edn")))))
(testing "that it adds a fallback-marker for build directories with no pipeline structure (e.g. because they were created before this feature was available)"
(let [home-dir (temp-util/create-temp-dir)]
(.mkdirs (io/file home-dir "build-1"))
(is (= {1 :fallback} (read-build-datas home-dir "pipeline-structure.edn")))))
(is (= {1 :fallback} (read-normal-build-data-from home-dir "pipeline-structure.edn")))))
(testing "that it will return empty data if build directory contains data but no build number"
(let [home-dir (temp-util/create-temp-dir)]
(.mkdirs (io/file home-dir "build-not-a-build-number"))
(is (= {} (read-build-datas home-dir "pipeline-structure.edn"))))))
(is (= {} (read-normal-build-data-from home-dir "pipeline-structure.edn"))))))

(defn- roundtrip-date-time [data]
(dates->clj-times
Expand Down

0 comments on commit b068d1f

Please sign in to comment.