From fd1c27448c7c2285a3ba9e2054404ab6afe091b4 Mon Sep 17 00:00:00 2001 From: davidvader Date: Fri, 3 Jan 2025 15:12:30 -0600 Subject: [PATCH 1/7] feat: render pipeline warnings as line annotations --- src/elm/Components/Logs.elm | 2 +- src/elm/Components/Svgs.elm | 17 +- src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm | 219 ++++++++++++++----- src/elm/Vela.elm | 108 ++++++++- src/scss/_main.scss | 4 +- src/scss/_pipelines.scss | 74 +++++++ 6 files changed, 358 insertions(+), 66 deletions(-) diff --git a/src/elm/Components/Logs.elm b/src/elm/Components/Logs.elm index b4ab5ff7a..289584679 100644 --- a/src/elm/Components/Logs.elm +++ b/src/elm/Components/Logs.elm @@ -194,7 +194,7 @@ viewLine pushUrlHashMsg resourceType resourceNumber shift focus logLine lineNumb ] [ span [] [ text <| String.fromInt lineNumber ] ] ] - , td [ class "break-text", class "overflow-auto" ] + , td [ class "break-text", class "overflow-auto", class "line-content" ] [ code [ Util.testAttribute <| String.join "-" [ "log", "data", resourceNumber, String.fromInt lineNumber ] ] [ logLine.view ] diff --git a/src/elm/Components/Svgs.elm b/src/elm/Components/Svgs.elm index 2b7f682c6..9543b58ae 100644 --- a/src/elm/Components/Svgs.elm +++ b/src/elm/Components/Svgs.elm @@ -4,7 +4,8 @@ SPDX-License-Identifier: Apache-2.0 module Components.Svgs exposing - ( buildStatusAnimation + ( annotationCircle + , buildStatusAnimation , buildStatusToIcon , buildVizLegendEdge , buildVizLegendNode @@ -792,3 +793,17 @@ buildVizLegendEdge attrs = ) [] ] + + +{-| annotationCircle : produces svg icon for line annotation bubble. +-} +annotationCircle : String -> Html msg +annotationCircle cls = + svg + [ class "-icon" + , class cls + , viewBox "0 0 48 48" + , width "22" + , height "22" + ] + [ Svg.circle [ cx "24", cy "24", r "8" ] [] ] diff --git a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm index 6fe1cc6cb..b9255b7cb 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm @@ -9,11 +9,12 @@ import Ansi.Log import Array import Auth import Components.Loading +import Components.Svgs import Dict exposing (Dict) import Effect exposing (Effect) import FeatherIcons import Html exposing (Html, a, button, code, details, div, small, span, strong, summary, table, td, text, tr) -import Html.Attributes exposing (attribute, class, href, id, target) +import Html.Attributes exposing (attribute, class, disabled, href, id, title) import Html.Events exposing (onClick) import Http import Http.Detailed @@ -220,18 +221,8 @@ update shared route msg model = sideEffect = case model.build of RemoteData.Success build -> - if expand then - Effect.expandPipelineConfig - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = GetExpandBuildPipelineConfigResponse { applyDomFocus = False } - , org = route.params.org - , repo = route.params.repo - , ref = build.commit - } - - else - Effect.getPipelineConfig + Effect.batch + [ Effect.getPipelineConfig { baseUrl = shared.velaAPIBaseURL , session = shared.session , onResponse = GetBuildPipelineConfigResponse { applyDomFocus = False } @@ -239,6 +230,19 @@ update shared route msg model = , repo = route.params.repo , ref = build.commit } + , if expand then + Effect.expandPipelineConfig + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = GetExpandBuildPipelineConfigResponse { applyDomFocus = False } + , org = route.params.org + , repo = route.params.repo + , ref = build.commit + } + + else + Effect.none + ] _ -> Effect.getBuild @@ -314,31 +318,16 @@ update shared route msg model = GetBuildResponse options response -> case response of Ok ( _, build ) -> - let - getPipelineConfigEffect = - if model.expand then - Effect.expandPipelineConfig - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = GetExpandBuildPipelineConfigResponse { applyDomFocus = options.applyDomFocus } - , org = route.params.org - , repo = route.params.repo - , ref = build.commit - } - - else - Effect.getPipelineConfig - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = GetBuildPipelineConfigResponse { applyDomFocus = options.applyDomFocus } - , org = route.params.org - , repo = route.params.repo - , ref = build.commit - } - in ( { model | build = RemoteData.Success build } , Effect.batch - [ getPipelineConfigEffect + [ Effect.getPipelineConfig + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = GetBuildPipelineConfigResponse { applyDomFocus = options.applyDomFocus } + , org = route.params.org + , repo = route.params.repo + , ref = build.commit + } , Effect.getPipelineTemplates { baseUrl = shared.velaAPIBaseURL , session = shared.session @@ -370,12 +359,26 @@ update shared route msg model = } , expanding = False } - , if Focus.canTarget model.focus && options.applyDomFocus then - FocusOn { target = Focus.toDomTarget model.focus } - |> Effect.sendMsg + , Effect.batch + [ if model.expand then + Effect.expandPipelineConfig + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = GetExpandBuildPipelineConfigResponse { applyDomFocus = options.applyDomFocus } + , org = route.params.org + , repo = route.params.repo + , ref = pipeline.commit + } - else - Effect.none + else + Effect.none + , if Focus.canTarget model.focus && options.applyDomFocus then + FocusOn { target = Focus.toDomTarget model.focus } + |> Effect.sendMsg + + else + Effect.none + ] ) Err error -> @@ -389,11 +392,16 @@ update shared route msg model = GetExpandBuildPipelineConfigResponse options response -> case response of Ok ( _, expandedPipeline ) -> + let + p = + RemoteData.withDefault Vela.defaultPipelineConfig model.pipeline + in ( { model | pipeline = - RemoteData.Success - { rawData = "" - , decodedData = expandedPipeline + RemoteData.succeed + { p + | rawData = "" + , decodedData = expandedPipeline } , expanding = False } @@ -486,7 +494,7 @@ view shared route model = else div [ class "icon" ] [ FeatherIcons.circle |> FeatherIcons.withSize 20 |> FeatherIcons.toHtml [] ] - , small [ class "tip" ] [ text "note: yaml fields will be sorted alphabetically when the pipeline is expanded." ] + , small [] [ text "note: yaml fields will be sorted alphabetically when the pipeline is expanded." ] ] _ -> @@ -522,7 +530,39 @@ view shared route model = in { title = "Pipeline" , body = - [ div [ class "pipeline" ] + [ case model.pipeline of + RemoteData.Success p -> + if List.length p.warnings > 0 then + div [ class "logs-container", class "-pipeline" ] + [ table + [ class "logs-table" + , class "pipeline" + ] + [ div [ class "header" ] + [ span [] + [ text "Warnings" + ] + ] + , if model.expand then + span [ class "tip" ] + [ small [] + [ text "note: this pipeline is expanded, the line numbers shown only apply to the base pipeline configuration, they are not accurate when viewing an expanded pipeline." ] + ] + + else + text "" + , div [ class "warnings" ] <| + List.map (viewWarningAsLogLine model shared) <| + List.sort p.warnings + ] + ] + + else + text "" + + _ -> + text "" + , div [ class "pipeline" ] [ case model.templates of RemoteData.Success templates -> if not <| Dict.isEmpty templates then @@ -566,6 +606,7 @@ view shared route model = [ div [ class "header" ] [ span [] [ text "Pipeline Configuration" + , RemoteData.unwrap (text "") (\p -> span [ class "commit" ] [ text p.commit ]) model.pipeline ] ] , div [ class "actions" ] @@ -576,7 +617,7 @@ view shared route model = RemoteData.Success pipeline -> if String.length pipeline.decodedData > 0 then div [ class "logs", Util.testAttribute "pipeline-configuration-data" ] <| - viewLines pipeline model.focus shared.shift + viewLines pipeline model.expand model.focus shared.shift else div [ class "no-pipeline" ] [ small [] [ code [] [ text "The pipeline found for this build/ref contains no data." ] ] ] @@ -660,17 +701,19 @@ viewTemplate ( _, t ) = {-| viewLines : creates a list of lines for a template. -} -viewLines : Vela.PipelineConfig -> Focus.Focus -> Bool -> List (Html Msg) -viewLines config focus shift = +viewLines : Vela.PipelineConfig -> Bool -> Focus.Focus -> Bool -> List (Html Msg) +viewLines config expand focus shiftKeyDown = config.decodedData |> Utils.Ansi.decodeAnsi |> Array.indexedMap (\idx line -> Just <| viewLine - shift + expand + shiftKeyDown (idx + 1) (Just line) + (Dict.get (idx + 1) config.lineWarnings) focus ) |> Array.toList @@ -679,11 +722,12 @@ viewLines config focus shift = {-| viewLine : creates a numbered, linkable line for a template. -} -viewLine : Bool -> Int -> Maybe Ansi.Log.Line -> Focus.Focus -> Html Msg -viewLine shiftKeyDown lineNumber line focus = +viewLine : Bool -> Bool -> Int -> Maybe Ansi.Log.Line -> Maybe (List String) -> Focus.Focus -> Html Msg +viewLine expand shiftKeyDown lineNumber line warnings focus = tr [ id <| String.fromInt lineNumber , class "line" + , Maybe.Extra.unwrap (class "") (\_ -> class "-warning") warnings ] [ case line of Just l -> @@ -692,7 +736,16 @@ viewLine shiftKeyDown lineNumber line focus = , Util.testAttribute <| String.join "-" [ "config", "line", String.fromInt lineNumber ] , class <| Focus.lineRangeStyles Nothing lineNumber focus ] - [ td [] + [ td + [ class "annotation" + , warnings + |> Maybe.Extra.filter (\_ -> not expand) + |> Maybe.Extra.unwrap (class "-hide") (\_ -> class "-show") + ] + [ Components.Svgs.annotationCircle "-warning" ] + , td + [ Html.Attributes.title <| "Jump to line " ++ String.fromInt lineNumber + ] [ button [ Util.onClickPreventDefault <| PushUrlHash @@ -709,10 +762,11 @@ viewLine shiftKeyDown lineNumber line focus = , class "button" , class "-link" , attribute "aria-label" "focus this line" + , title <| "Focus line " ++ String.fromInt lineNumber ] [ span [] [ text <| String.fromInt lineNumber ] ] ] - , td [ class "break-text", class "overflow-auto" ] + , td [ class "break-text", class "overflow-auto", class "line-content" ] [ code [ Util.testAttribute <| String.join "-" [ "config", "data", String.fromInt lineNumber ] ] [ Ansi.Log.viewLine l ] @@ -722,3 +776,60 @@ viewLine shiftKeyDown lineNumber line focus = Nothing -> text "" ] + + +{-| viewWarningAsLogLine : renders a warning as a log line. +-} +viewWarningAsLogLine : Model -> Shared.Model -> String -> Html Msg +viewWarningAsLogLine model shared warning = + let + ( maybeLineNumber, content ) = + Vela.lineNumberWarningfromWarningString warning + in + tr [ class "warning" ] + [ td [ class "annotation", class "-show" ] + [ Components.Svgs.annotationCircle "-warning" + ] + , td [] + [ case maybeLineNumber of + Just lineNumber -> + let + -- control focus behaviour so that the button always snaps to the right page location + focusChanged = + Maybe.Extra.unwrap False (\a -> a == lineNumber) model.focus.a + in + button + [ Util.onClickPreventDefault <| + if model.expand then + NoOp + + else if focusChanged then + FocusOn { target = Focus.toDomTarget { group = Nothing, a = Just lineNumber, b = Nothing } } + + else + PushUrlHash + { hash = + Focus.toString <| Focus.updateLineRange shared.shift Nothing lineNumber model.focus + } + , Util.testAttribute <| String.join "-" [ "warning", "line", "num", String.fromInt lineNumber ] + , class "button" + , class "-link" + , if model.expand then + class "-disabled" + + else + class "" + , class "line-number" + , attribute "aria-label" "jump to this line" + , title <| "Jump to line " ++ String.fromInt lineNumber + , disabled model.expand + ] + [ span [] [ text <| String.fromInt lineNumber ] ] + + Nothing -> + span [ class "no-line-number" ] [ text "-" ] + ] + , td [ class "line-content" ] + [ text content + ] + ] diff --git a/src/elm/Vela.elm b/src/elm/Vela.elm index 8f7d61f86..c181fe601 100644 --- a/src/elm/Vela.elm +++ b/src/elm/Vela.elm @@ -87,6 +87,7 @@ module Vela exposing , defaultCompilerPayload , defaultDeploymentPayload , defaultEnabledAllowEvents + , defaultPipelineConfig , defaultQueuePayload , defaultRepoPayload , defaultSchedulePayload @@ -103,6 +104,7 @@ module Vela exposing , encodeSettingsPayload , encodeUpdateUser , getAllowEventField + , lineNumberWarningfromWarningString , platformSettingsFieldUpdateToResponseConfig , repoFieldUpdateToResponseConfig , secretToKey @@ -114,7 +116,7 @@ module Vela exposing import Bytes.Encode import Dict exposing (Dict) import Json.Decode exposing (Decoder, andThen, bool, int, string, succeed) -import Json.Decode.Extra exposing (dict2, optionalField) +import Json.Decode.Extra exposing (dict2) import Json.Decode.Pipeline exposing (hardcoded, optional, required) import Json.Encode import RemoteData exposing (WebData) @@ -1109,8 +1111,24 @@ allowEventsFilterQueryKeys = type alias PipelineConfig = - { rawData : String + { commit : String + , flavor : String + , platform : String + , ref : String + , type_ : String + , version : String + , externalSecrets : Bool + , internalSecrets : Bool + , services : Bool + , stages : Bool + , steps : Bool + , templates : Bool + , warnings : List String + + -- decoded values + , rawData : String , decodedData : String + , lineWarnings : Dict Int (List String) } @@ -1126,16 +1144,90 @@ type alias Templates = Dict String Template +defaultPipelineConfig : PipelineConfig +defaultPipelineConfig = + PipelineConfig + "" + "" + "" + "" + "" + "" + False + False + False + False + False + False + [] + "" + "" + Dict.empty + + +lineNumberWarningfromWarningString : String -> ( Maybe Int, String ) +lineNumberWarningfromWarningString warning = + case String.split ":" warning of + prefix :: content -> + case String.toInt prefix of + Just lineNumber -> + ( Just lineNumber, String.join "" content ) + + Nothing -> + ( Nothing, warning ) + + _ -> + ( Nothing, warning ) + + decodePipelineConfig : Json.Decode.Decoder PipelineConfig decodePipelineConfig = Json.Decode.succeed - (\data -> - PipelineConfig - data - -- "decodedData" - "" - ) + PipelineConfig + |> optional "commit" string "" + |> optional "flavor" string "" + |> optional "platform" string "" + |> optional "ref" string "" + |> optional "type" string "" + |> optional "version" string "" + |> optional "external_secrets" bool False + |> optional "internal_secrets" bool False + |> optional "services" bool False + |> optional "stages" bool False + |> optional "steps" bool False + |> optional "templates" bool False + |> optional "warnings" (Json.Decode.list string) [] |> optional "data" string "" + |> optional "decodedData" string "" + |> optional "warnings" + (Json.Decode.list string + |> Json.Decode.andThen + (\warnings -> + Json.Decode.succeed + (List.foldl + (\warning dict -> + case lineNumberWarningfromWarningString warning of + ( Just line, w ) -> + Dict.update line + (\maybeWarnings -> + case maybeWarnings of + Just existingWarnings -> + Just (w :: existingWarnings) + + Nothing -> + Just [ w ] + ) + dict + + _ -> + dict + ) + Dict.empty + warnings + ) + ) + ) + Dict.empty decodePipelineExpand : Json.Decode.Decoder String diff --git a/src/scss/_main.scss b/src/scss/_main.scss index 161127bad..0ee19d409 100644 --- a/src/scss/_main.scss +++ b/src/scss/_main.scss @@ -1092,7 +1092,7 @@ details.build-toggle { .logs .line { display: flex; align-items: flex-start; - margin: 0 0.5em; + margin: 0 0.5em 0 0; .no-data { margin-left: 1rem; @@ -1138,7 +1138,7 @@ details.build-toggle { scrollbar-width: thin; } -.logs-table tr td:nth-child(2) { +.logs-table .line-content { padding-left: 1rem; } diff --git a/src/scss/_pipelines.scss b/src/scss/_pipelines.scss index 12c0a2fb7..ac3f0d6b6 100644 --- a/src/scss/_pipelines.scss +++ b/src/scss/_pipelines.scss @@ -24,6 +24,31 @@ border-left: 1px solid var(--color-green); } } + + .commit { + margin: 0.2rem 0.6rem; + padding: 0.4rem 0.4rem; + + color: var(--color-text); + font-size: 0.9rem; + + background-color: var(--color-bg); + } + + .tip { + display: flex; + padding-bottom: 0.4rem; + + border-bottom: 1px solid var(--color-bg-light); + + small { + display: inline-block; + margin: 0.4rem 0.4rem 0 0.4rem; + padding: 0.2rem 0.4rem; + + background-color: var(--color-bg-light); + } + } } .templates { @@ -38,6 +63,7 @@ padding-right: 1rem; padding-left: 1rem; } + margin-bottom: 1rem; padding: 0.8rem 0 0.8rem 0; @@ -180,3 +206,51 @@ padding-bottom: 1rem; } } + +.pipeline .warnings { + display: flex; + flex-direction: column; + margin-bottom: 0.8rem; + padding: 0.6rem 0.4rem 0.6rem 0; + + .no-line-number { + display: block; + width: 6ch; + + font-family: var(--font-code); + text-align: right; + } +} + +tr.warning, +.line.-warning .wrapper { + position: relative; +} + +.warnings .-link.-disabled { + color: var(--color-bg-light); +} + +.annotation { + padding: 0; + + .-warning { + fill: var(--color-yellow); + stroke: var(--color-yellow); + } + + &.-hide { + visibility: hidden; + } + + &.-show:before { + position: absolute; + + width: 1px; + height: 100%; + + background-color: var(--color-yellow); + + content: ''; + } +} From c8071a5c8d66ec60f30f7c399953e373b9ca5e0b Mon Sep 17 00:00:00 2001 From: davidvader Date: Fri, 3 Jan 2025 15:20:51 -0600 Subject: [PATCH 2/7] chore: cleanup --- src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm | 4 +- src/elm/Vela.elm | 75 ++++++++------------ 2 files changed, 33 insertions(+), 46 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm index b9255b7cb..b070bcb6e 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm @@ -393,13 +393,13 @@ update shared route msg model = case response of Ok ( _, expandedPipeline ) -> let - p = + basePipeline = RemoteData.withDefault Vela.defaultPipelineConfig model.pipeline in ( { model | pipeline = RemoteData.succeed - { p + { basePipeline | rawData = "" , decodedData = expandedPipeline } diff --git a/src/elm/Vela.elm b/src/elm/Vela.elm index c181fe601..83c28e794 100644 --- a/src/elm/Vela.elm +++ b/src/elm/Vela.elm @@ -1132,6 +1132,11 @@ type alias PipelineConfig = } +defaultPipelineConfig : PipelineConfig +defaultPipelineConfig = + PipelineConfig "" "" "" "" "" "" False False False False False False [] "" "" Dict.empty + + type alias Template = { link : String , name : String @@ -1144,27 +1149,6 @@ type alias Templates = Dict String Template -defaultPipelineConfig : PipelineConfig -defaultPipelineConfig = - PipelineConfig - "" - "" - "" - "" - "" - "" - False - False - False - False - False - False - [] - "" - "" - Dict.empty - - lineNumberWarningfromWarningString : String -> ( Maybe Int, String ) lineNumberWarningfromWarningString warning = case String.split ":" warning of @@ -1202,32 +1186,35 @@ decodePipelineConfig = |> optional "warnings" (Json.Decode.list string |> Json.Decode.andThen - (\warnings -> - Json.Decode.succeed - (List.foldl - (\warning dict -> - case lineNumberWarningfromWarningString warning of - ( Just line, w ) -> - Dict.update line - (\maybeWarnings -> - case maybeWarnings of - Just existingWarnings -> - Just (w :: existingWarnings) - - Nothing -> - Just [ w ] - ) - dict - - _ -> - dict - ) - Dict.empty - warnings + decodeAndCollapsePipelineWarnings + ) + Dict.empty + + +decodeAndCollapsePipelineWarnings : List String -> Json.Decode.Decoder (Dict Int (List String)) +decodeAndCollapsePipelineWarnings warnings = + Json.Decode.succeed + (List.foldl + (\warning dict -> + case lineNumberWarningfromWarningString warning of + ( Just line, w ) -> + Dict.update line + (\maybeWarnings -> + case maybeWarnings of + Just existingWarnings -> + Just (w :: existingWarnings) + + Nothing -> + Just [ w ] ) - ) + dict + + _ -> + dict ) Dict.empty + warnings + ) decodePipelineExpand : Json.Decode.Decoder String From 156755bc9657ad8d07a75ee8c80cc2e7fa09e20f Mon Sep 17 00:00:00 2001 From: davidvader Date: Tue, 7 Jan 2025 10:56:44 -0600 Subject: [PATCH 3/7] fix: address feedback --- src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm | 5 +-- src/elm/Utils/Warnings.elm | 32 ++++++++++++++++++++ src/elm/Vela.elm | 23 ++++---------- 3 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 src/elm/Utils/Warnings.elm diff --git a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm index b070bcb6e..d6358ceff 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm @@ -29,6 +29,7 @@ import Utils.Ansi import Utils.Errors as Errors import Utils.Focus as Focus import Utils.Helpers as Util +import Utils.Warnings as Warnings import Vela import View exposing (View) @@ -783,8 +784,8 @@ viewLine expand shiftKeyDown lineNumber line warnings focus = viewWarningAsLogLine : Model -> Shared.Model -> String -> Html Msg viewWarningAsLogLine model shared warning = let - ( maybeLineNumber, content ) = - Vela.lineNumberWarningfromWarningString warning + { maybeLineNumber, content } = + Warnings.fromString warning in tr [ class "warning" ] [ td [ class "annotation", class "-show" ] diff --git a/src/elm/Utils/Warnings.elm b/src/elm/Utils/Warnings.elm new file mode 100644 index 000000000..8a23e366b --- /dev/null +++ b/src/elm/Utils/Warnings.elm @@ -0,0 +1,32 @@ +{-- +SPDX-License-Identifier: Apache-2.0 +--} + + +module Utils.Warnings exposing (Warning, fromString) + +{-| Warning : an object that represents a point of focus. +-} + + +type alias Warning = + { maybeLineNumber : Maybe Int + , content : String + } + + +{-| fromString : parses a warning string into a line number and a message. +-} +fromString : String -> Warning +fromString warning = + case String.split ":" warning of + prefix :: content -> + case String.toInt prefix of + Just lineNumber -> + { maybeLineNumber = Just lineNumber, content = String.concat content } + + Nothing -> + { maybeLineNumber = Nothing, content = warning } + + _ -> + { maybeLineNumber = Nothing, content = warning } diff --git a/src/elm/Vela.elm b/src/elm/Vela.elm index 83c28e794..32deb92eb 100644 --- a/src/elm/Vela.elm +++ b/src/elm/Vela.elm @@ -104,7 +104,6 @@ module Vela exposing , encodeSettingsPayload , encodeUpdateUser , getAllowEventField - , lineNumberWarningfromWarningString , platformSettingsFieldUpdateToResponseConfig , repoFieldUpdateToResponseConfig , secretToKey @@ -120,6 +119,7 @@ import Json.Decode.Extra exposing (dict2) import Json.Decode.Pipeline exposing (hardcoded, optional, required) import Json.Encode import RemoteData exposing (WebData) +import Utils.Warnings as Warnings @@ -1149,21 +1149,6 @@ type alias Templates = Dict String Template -lineNumberWarningfromWarningString : String -> ( Maybe Int, String ) -lineNumberWarningfromWarningString warning = - case String.split ":" warning of - prefix :: content -> - case String.toInt prefix of - Just lineNumber -> - ( Just lineNumber, String.join "" content ) - - Nothing -> - ( Nothing, warning ) - - _ -> - ( Nothing, warning ) - - decodePipelineConfig : Json.Decode.Decoder PipelineConfig decodePipelineConfig = Json.Decode.succeed @@ -1196,7 +1181,11 @@ decodeAndCollapsePipelineWarnings warnings = Json.Decode.succeed (List.foldl (\warning dict -> - case lineNumberWarningfromWarningString warning of + let + { maybeLineNumber, content } = + Warnings.fromString warning + in + case ( maybeLineNumber, content ) of ( Just line, w ) -> Dict.update line (\maybeWarnings -> From 41c796cf82cfbd8364604de8fd05d543dd46ee23 Mon Sep 17 00:00:00 2001 From: davidvader Date: Tue, 7 Jan 2025 12:32:11 -0600 Subject: [PATCH 4/7] feat: cypress tests --- cypress/fixtures/pipeline_warnings.json | 18 +++ cypress/integration/pipeline.spec.js | 125 +++++++++++++++++++ cypress/support/commands.js | 10 ++ src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm | 26 ++-- 4 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 cypress/fixtures/pipeline_warnings.json diff --git a/cypress/fixtures/pipeline_warnings.json b/cypress/fixtures/pipeline_warnings.json new file mode 100644 index 000000000..4e387caff --- /dev/null +++ b/cypress/fixtures/pipeline_warnings.json @@ -0,0 +1,18 @@ +{ + "id": 1, + "repo_id": 1, + "commit": "9b1d8bded6e992ab660eaee527c5e3232d0a2441", + "flavor": "", + "platform": "", + "ref": "refs/heads/main", + "type": "yaml", + "version": "1", + "external_secrets": false, + "internal_secrets": false, + "services": false, + "stages": false, + "steps": true, + "templates": true, + "warnings": ["pipeline is using shared secrets", "4:invalid template name"], + "data": "dmVyc2lvbjogIjEiCnN0ZXBzOgotIHRlbXBsYXRlOgogICAgbmFtZTogY3VzdG9tX3RlbXBsYXRlCiAgICB2YXJzOgogICAgICBidWlsZF9ldmVudDogJHtCVUlMRF9FVkVOVH0KICBuYW1lOiBpbnZva2UgdGVtcGxhdGUxX25hbWUKICBwdWxsOiBub3RfcHJlc2VudAotIHRlbXBsYXRlOgogICAgbmFtZTogc2ltcGxlX3RlbXBsYXRlCiAgICB2YXJzOgogICAgICBidWlsZF9ldmVudDogJHtCVUlMRF9FVkVOVH0KICBuYW1lOiBpbnZva2UgdGVtcGxhdGUyX25hbWUKICBwdWxsOiBub3RfcHJlc2VudAotIHRlbXBsYXRlOgogICAgbmFtZTogYnJhbmNoZWRfdGVtcGxhdGUKICAgIHZhcnM6CiAgICAgIGJ1aWxkX2V2ZW50OiAke0JVSUxEX0VWRU5UfQogIG5hbWU6IGludm9rZSB0ZW1wbGF0ZTNfbmFtZQogIHB1bGw6IG5vdF9wcmVzZW50CnRlbXBsYXRlczoKLSBuYW1lOiB0ZW1wbGF0ZTFfbmFtZQogIHNvdXJjZTogZ2l0aHViLmNvbS9naXRodWIvb2N0b2NhdC90ZW1wbGF0ZTEueW1sCiAgdHlwZTogZ2l0aHViCi0gbmFtZTogdGVtcGxhdGUyX25hbWUKICBzb3VyY2U6IGdpdGh1Yi5jb20vZ2l0aHViL29jdG9jYXQvdGVtcGxhdGUyLnltbAogIHR5cGU6IGdpdGh1YgotIG5hbWU6IHRlbXBsYXRlM19uYW1lCiAgc291cmNlOiBnaXRodWIuY29tL2dpdGh1Yi9vY3RvY2F0L3RlbXBsYXRlMy55bWwKICB0eXBlOiBnaXRodWI=" +} diff --git a/cypress/integration/pipeline.spec.js b/cypress/integration/pipeline.spec.js index 3ffcfd906..560079bc1 100644 --- a/cypress/integration/pipeline.spec.js +++ b/cypress/integration/pipeline.spec.js @@ -90,6 +90,10 @@ context('Pipeline', () => { cy.get('[data-test=pipeline-expand]').should('exist'); }); + it('warnings should not be visible', () => { + cy.get('[data-test=pipeline-warnings]').should('not.be.visible'); + }); + context('click expand templates', () => { beforeEach(() => { cy.get('[data-test=pipeline-expand-toggle]').click({ @@ -285,4 +289,125 @@ context('Pipeline', () => { }); }, ); + context( + 'logged in and server returning valid pipeline configuration (with warnings) and templates', + () => { + beforeEach(() => { + cy.server(); + cy.stubBuild(); + cy.stubPipelineWithWarnings(); + cy.stubPipelineExpand(); + cy.stubPipelineTemplates(); + cy.login('/github/octocat/1/pipeline'); + }); + + it('warnings should be visible', () => { + cy.get('[data-test=pipeline-warnings]').should('be.visible'); + }); + + it('should show 2 warnings', () => { + cy.get('[data-test=pipeline-warnings]') + .children() + .should('have.length', 2); + }); + + it('warning with line number should show line number button', () => { + cy.get('[data-test=warning-line-num-4]') + .should('be.visible') + .should('not.have.class', '-disabled'); + }); + + it('warning with line number should show content without line number', () => { + cy.get('[data-test=warning-0] .line-content') + .should('be.visible') + .should('not.contain', '4') + .should('contain', 'template'); + }); + + it('warning without line number should replace button with dash', () => { + cy.get('[data-test=warning-1]') + .should('be.visible') + .should('contain', '-'); + }); + + it('warning without line number should content', () => { + cy.get('[data-test=warning-1] .line-content') + .should('be.visible') + .should('contain', 'secrets'); + }); + + it('log line with warning should show annotation', () => { + cy.get('[data-test=warning-annotation-line-4]').should('be.visible'); + }); + + it('other lines should not show annotations', () => { + cy.get('[data-test=warning-annotation-line-5]').should( + 'not.be.visible', + ); + }); + + context('click warning line number', () => { + beforeEach(() => { + cy.get('[data-test=warning-line-num-4]').click({ force: true }); + }); + + it('should update path with line num', () => { + cy.hash().should('eq', '#4'); + }); + + it('should set focus style on single line', () => { + cy.get('[data-test=config-line-4]').should('have.class', '-focus'); + }); + + it('other lines should not have focus style', () => { + cy.get('[data-test=config-line-3]').should( + 'not.have.class', + '-focus', + ); + }); + }); + + context('click expand templates', () => { + beforeEach(() => { + cy.get('[data-test=pipeline-expand-toggle]').click({ + force: true, + }); + cy.wait('@expand'); + }); + + it('should update path with expand query', () => { + cy.location().should(loc => { + expect(loc.search).to.eq('?expand=true'); + }); + }); + + it('should show pipeline expansion note', () => { + cy.get('[data-test=pipeline-warnings-expand-note]').contains('note'); + }); + + it('warning with line number should show disabled line number button', () => { + cy.get('[data-test=warning-line-num-4]') + .should('be.visible') + .should('have.class', '-disabled'); + }); + + context('click warning line number', () => { + beforeEach(() => { + cy.get('[data-test=warning-line-num-4]').click({ force: true }); + }); + + it('should not update path with line num', () => { + cy.hash().should('not.eq', '#4'); + }); + + it('other lines should not have focus style', () => { + cy.get('[data-test=config-line-3]').should( + 'not.have.class', + '-focus', + ); + }); + }); + }); + }, + ); }); diff --git a/cypress/support/commands.js b/cypress/support/commands.js index ed6de0ea5..8b131522c 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -531,6 +531,16 @@ Cypress.Commands.add('stubPipeline', () => { }); }); +Cypress.Commands.add('stubPipelineWithWarnings', () => { + cy.fixture('pipeline_warnings.json').as('pipeline'); + cy.route({ + method: 'GET', + url: '*api/v1/pipelines/*/*/*', + status: 200, + response: '@pipeline', + }); +}); + Cypress.Commands.add('stubPipelineErrors', () => { cy.route({ method: 'GET', diff --git a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm index d6358ceff..32a18df28 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Pipeline.elm @@ -534,7 +534,10 @@ view shared route model = [ case model.pipeline of RemoteData.Success p -> if List.length p.warnings > 0 then - div [ class "logs-container", class "-pipeline" ] + div + [ class "logs-container" + , class "-pipeline" + ] [ table [ class "logs-table" , class "pipeline" @@ -545,15 +548,15 @@ view shared route model = ] ] , if model.expand then - span [ class "tip" ] + span [ class "tip", Util.testAttribute "pipeline-warnings-expand-note" ] [ small [] [ text "note: this pipeline is expanded, the line numbers shown only apply to the base pipeline configuration, they are not accurate when viewing an expanded pipeline." ] ] else text "" - , div [ class "warnings" ] <| - List.map (viewWarningAsLogLine model shared) <| + , div [ class "warnings", Util.testAttribute "pipeline-warnings" ] <| + List.indexedMap (viewWarningAsLogLine model shared) <| List.sort p.warnings ] ] @@ -739,6 +742,7 @@ viewLine expand shiftKeyDown lineNumber line warnings focus = ] [ td [ class "annotation" + , Util.testAttribute <| String.join "-" [ "warning", "annotation", "line", String.fromInt lineNumber ] , warnings |> Maybe.Extra.filter (\_ -> not expand) |> Maybe.Extra.unwrap (class "-hide") (\_ -> class "-show") @@ -781,13 +785,16 @@ viewLine expand shiftKeyDown lineNumber line warnings focus = {-| viewWarningAsLogLine : renders a warning as a log line. -} -viewWarningAsLogLine : Model -> Shared.Model -> String -> Html Msg -viewWarningAsLogLine model shared warning = +viewWarningAsLogLine : Model -> Shared.Model -> Int -> String -> Html Msg +viewWarningAsLogLine model shared idx warning = let { maybeLineNumber, content } = Warnings.fromString warning in - tr [ class "warning" ] + tr + [ class "warning" + , Util.testAttribute <| String.join "-" [ "warning", String.fromInt idx ] + ] [ td [ class "annotation", class "-show" ] [ Components.Svgs.annotationCircle "-warning" ] @@ -828,7 +835,10 @@ viewWarningAsLogLine model shared warning = [ span [] [ text <| String.fromInt lineNumber ] ] Nothing -> - span [ class "no-line-number" ] [ text "-" ] + span + [ class "no-line-number" + ] + [ text "-" ] ] , td [ class "line-content" ] [ text content From e6482a54e4d2e400c2001180b2d66e5690804275 Mon Sep 17 00:00:00 2001 From: davidvader Date: Tue, 7 Jan 2025 12:36:55 -0600 Subject: [PATCH 5/7] fix: comment spacing elm-format hack --- src/elm/Utils/Warnings.elm | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/elm/Utils/Warnings.elm b/src/elm/Utils/Warnings.elm index 8a23e366b..e7f98c17b 100644 --- a/src/elm/Utils/Warnings.elm +++ b/src/elm/Utils/Warnings.elm @@ -3,18 +3,33 @@ SPDX-License-Identifier: Apache-2.0 --} -module Utils.Warnings exposing (Warning, fromString) +module Utils.Warnings exposing + ( Warning + , fromString + ) + +{-| + +@docs Warning +@docs fromString -{-| Warning : an object that represents a point of focus. -} +-- TYPES + +{-| Warning : an object that represents a point of focus. +-} type alias Warning = { maybeLineNumber : Maybe Int , content : String } + +-- HELPERS + + {-| fromString : parses a warning string into a line number and a message. -} fromString : String -> Warning From ac019685136c04e33cd80e8e5d813106aa971f77 Mon Sep 17 00:00:00 2001 From: davidvader Date: Tue, 7 Jan 2025 12:40:19 -0600 Subject: [PATCH 6/7] fix: scss organization --- src/scss/_pipelines.scss | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/scss/_pipelines.scss b/src/scss/_pipelines.scss index ac3f0d6b6..76700f1c5 100644 --- a/src/scss/_pipelines.scss +++ b/src/scss/_pipelines.scss @@ -212,14 +212,14 @@ flex-direction: column; margin-bottom: 0.8rem; padding: 0.6rem 0.4rem 0.6rem 0; +} - .no-line-number { - display: block; - width: 6ch; +.warning .no-line-number { + display: block; + width: 6ch; - font-family: var(--font-code); - text-align: right; - } + font-family: var(--font-code); + text-align: right; } tr.warning, From 86ab801d9a49b80ba2694e22267afdd2aa4689c9 Mon Sep 17 00:00:00 2001 From: davidvader Date: Tue, 7 Jan 2025 14:41:07 -0600 Subject: [PATCH 7/7] fix: disabled link constrast on light-theme --- src/scss/_pipelines.scss | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/scss/_pipelines.scss b/src/scss/_pipelines.scss index 76700f1c5..7b457e77a 100644 --- a/src/scss/_pipelines.scss +++ b/src/scss/_pipelines.scss @@ -227,10 +227,14 @@ tr.warning, position: relative; } -.warnings .-link.-disabled { +.warnings .-disabled { color: var(--color-bg-light); } +.theme-light .warnings .-disabled { + color: var(--color-coal-light); +} + .annotation { padding: 0;