From 49e13d0fca0b130216953786e50c35e26450a961 Mon Sep 17 00:00:00 2001 From: Florian Sellmayr Date: Sun, 16 Oct 2016 21:26:03 +0200 Subject: [PATCH] Implement fallback for builds without stored pipeline-structure, displaying warning in UI #131, #6 --- .../default_pipeline_state_persistence.clj | 6 ++-- src/clj/lambdacd/state/core.clj | 23 +++++++++++++-- src/cljs/lambdacd/pipeline.cljs | 8 +++++- src/less/app.less | 12 +++++++- ...efault_pipeline_state_persistence_test.clj | 4 +-- test/clj/lambdacd/state/core_test.clj | 28 +++++++++++++++---- 6 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/clj/lambdacd/internal/default_pipeline_state_persistence.clj b/src/clj/lambdacd/internal/default_pipeline_state_persistence.clj index 06b0a964..688762c9 100644 --- a/src/clj/lambdacd/internal/default_pipeline_state_persistence.clj +++ b/src/clj/lambdacd/internal/default_pipeline_state_persistence.clj @@ -98,12 +98,14 @@ (spit f (pr-str pipeline-structure)))) (defn- read-pipeline-structure-edn [f] - {(build-number-from-path f) (edn/read-string (slurp 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-pipeline-structures [home-dir] (->> (build-dirs home-dir) (map pipeline-structure-path) - (filter file-exists?) (map read-pipeline-structure-edn) (into {}))) diff --git a/src/clj/lambdacd/state/core.clj b/src/clj/lambdacd/state/core.clj index e070cea3..dce8ab4c 100644 --- a/src/clj/lambdacd/state/core.clj +++ b/src/clj/lambdacd/state/core.clj @@ -64,10 +64,29 @@ (get (get-step-results ctx build-number) step-id)) +(defn- annotated-step [step] + (let [annotate-children (fn [x] + (if (:children x) + (assoc x :children (map annotated-step (:children x))) + x))] + (-> step + (assoc :pipeline-structure-fallback true) + (annotate-children)))) + +(defn- annotated-fallback-structure [ctx] + (let [current-structure (pipeline-structure/pipeline-display-representation (:pipeline-def ctx))] + (map annotated-step current-structure))) + +(defn- stored-structure-or-fallback [ctx build-number] + (let [stored-structure (protocols/get-pipeline-structure (state-component ctx) build-number)] + (if (= :fallback stored-structure) + (annotated-fallback-structure ctx) + stored-structure))) + (defn get-pipeline-structure "Returns a map describing the structure of the pipeline" [ctx build-number] (let [component (state-component ctx)] (if (satisfies? protocols/PipelineStructureSource component) - (protocols/get-pipeline-structure component build-number) - (pipeline-structure/pipeline-display-representation (:pipeline-def ctx))))) + (stored-structure-or-fallback ctx build-number) + (annotated-fallback-structure ctx)))) diff --git a/src/cljs/lambdacd/pipeline.cljs b/src/cljs/lambdacd/pipeline.cljs index 5e387c30..a73d3a07 100644 --- a/src/cljs/lambdacd/pipeline.cljs +++ b/src/cljs/lambdacd/pipeline.cljs @@ -189,13 +189,19 @@ [:li [:a {:class (classes "pipeline__controls__control" (control-active-if @expand-active?)) :on-click #(re-frame/dispatch [::db/toggle-expand-active])} "Expand active"]] [:li [:a {:class (classes "pipeline__controls__control" (control-active-if @expand-failures?)) :on-click #(re-frame/dispatch [::db/toggle-expand-failures])} "Expand failures"]]]))) +(defn old-pipeline-structure-warning [build-steps] + (if (:pipeline-structure-fallback (first build-steps)) + [:ul {:class "pipeline__controls"} + [:li {:class "pipeline__controls__control warning"} [:i {:class "fa fa-exclamation warning-icon"}] "This is an old build with no stored pipeline structure. Using current structure as a best guess. Display and retriggering might not work as expected."]])) + (defn pipeline-component [] (let [build-state-atom (re-frame/subscribe [::db/pipeline-state]) build-number-subscription (re-frame/subscribe [::db/build-number])] (fn [] [:div {:class "pipeline" :key "build-pipeline"} [pipeline-controls] + [old-pipeline-structure-warning @build-state-atom] [:ol {:class "pipeline__step-container pipeline__step-container--sequential"} (doall (for [step @build-state-atom] - ^{:key (:step-id step)} [build-step step @build-number-subscription]))]]))) \ No newline at end of file + ^{:key (:step-id step)} [build-step step @build-number-subscription]))]]))) diff --git a/src/less/app.less b/src/less/app.less index d56fa565..a37dd534 100644 --- a/src/less/app.less +++ b/src/less/app.less @@ -47,4 +47,14 @@ display: flex; justify-content: center; align-items: center; -} \ No newline at end of file +} + +.warning-icon { + padding-top: 5px; + padding-bottom: 5px; + padding-right: 5px; +} + +.warning { + color: @base-orange; +} diff --git a/test/clj/lambdacd/internal/default_pipeline_state_persistence_test.clj b/test/clj/lambdacd/internal/default_pipeline_state_persistence_test.clj index dcc6a9d3..c8c6d5a2 100644 --- a/test/clj/lambdacd/internal/default_pipeline_state_persistence_test.clj +++ b/test/clj/lambdacd/internal/default_pipeline_state_persistence_test.clj @@ -74,10 +74,10 @@ (testing "that it will return an empty data if no state has been written yet" (let [home-dir (utils/create-temp-dir)] (is (= {} (read-pipeline-structures home-dir))))) - (testing "that it ignores build directories with no pipeline structure (e.g. because they were created before this feature was available)" + (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 (utils/create-temp-dir)] (.mkdirs (io/file home-dir "build-1")) - (is (= {} (read-pipeline-structures home-dir)))))) + (is (= {1 :fallback} (read-pipeline-structures home-dir)))))) (defn- roundtrip-date-time [data] (dates->clj-times diff --git a/test/clj/lambdacd/state/core_test.clj b/test/clj/lambdacd/state/core_test.clj index 80e463cb..45dc155f 100644 --- a/test/clj/lambdacd/state/core_test.clj +++ b/test/clj/lambdacd/state/core_test.clj @@ -10,7 +10,22 @@ (def some-step-id [0]) (def some-step-result {:foo :bat}) (def some-structure {:some :structure}) -(def some-pipeline-def `(foo)) + +(defn ^{:display-type :container} foo [& _]) +(defn bar [& _]) +(def some-pipeline-def `((foo + bar))) + +(def some-pipeline-def-structure [{:name "foo" + :type :container + :has-dependencies false + :pipeline-structure-fallback true + :step-id `(1) + :children [{:name "bar" + :type :step + :has-dependencies false + :pipeline-structure-fallback true + :step-id `(1 1)}]}]) (deftest consume-step-result-update-test (testing "that calls to a StepResultUpdateConsumer will just pass through" @@ -85,8 +100,9 @@ (is (received? component state-protocols/get-pipeline-structure [1])))) (testing "that we get the current pipeline structure if the component doesn't support PipelineStructures" (let [component (mock state-protocols/QueryStepResultsSource {:get-step-results {:some :step-results}})] - (is (= [{:name "foo" - :type :unknown - :has-dependencies false - :step-id `(1)}] (s/get-pipeline-structure (some-ctx-with :pipeline-state-component component - :pipeline-def some-pipeline-def) 1)))))) + (is (= some-pipeline-def-structure (s/get-pipeline-structure (some-ctx-with :pipeline-state-component component + :pipeline-def some-pipeline-def) 1))))) + (testing "that we get the current pipeline structure if the component returns :fallback and annotate the structure accordingly" + (let [component (mock state-protocols/PipelineStructureSource {:get-pipeline-structure :fallback})] + (is (= some-pipeline-def-structure (s/get-pipeline-structure (some-ctx-with :pipeline-state-component component + :pipeline-def some-pipeline-def) 1))))))