datreeio / helm-datree

A Helm plugin to validate charts against the Datree's CLI tool
https://hub.datree.io/integrations
MIT License
113 stars 26 forks source link

CI/CD friendliness #5

Closed wind57 closed 3 years ago

wind57 commented 3 years ago

An interesting tool overall!

I do not see a way to use it in a CI/CD env, unless I am missing the obvious.

It would be great if it had something along the lines of --quiet and --error-count, so that I could parse the result of helm datree test ... and find our if I need to fail the build or not. Currently this is very cumbersome to do (I get the output and parse it, which to put it mildly, simply sucks).

Thank you for looking into this.

wind57 commented 3 years ago

oh! so I can pass -o json to helm datree... this simplifies things. Though the output is weird:

  "InvalidK8sFiles": [
    {
      "Path": "/tmp/manifest.yaml",
      "ValidationErrors": [
        {
          "ErrorMessage": "For field apiVersion: apiVersion must be one of the following: \"rbac.authorization.k8s.io/v1\""
        }
      ]
    }
  ]

This is what I currently ended up doing :

IFS=$'\n'
declare -a datree_result
datree_result=$(helm datree test src/main/helm -o json | jq -c 'paths | select(.[-1] == "ErrorMessage")')

if [[ "${datree_result[@]}" =~ "ErrorMessage" ]]
then
   echo "failure"
   exit 1
fi
eyarz commented 3 years ago

@wind57 I forgot to put myself as a watcher to this repo, so I missed your issue - sorry about that :disappointed:

I'm glad to see that you find a workaround, but I still want to answer your questions:

I'm wondering why do parse Datree's output? If it fails, it should have exit code 1 anyway...

dimabru commented 3 years ago

FYI, our latest update includes a small change to the way we generate these tmp files. Since we added pipe support in datree cli, we can now forward helm template output directly to the datree cli. This results in a slightly different filename in tmp folder. Should be something like /var/folders/4k/7h6k03k11rv8gy62py4fxqxh0000gm/T/datree_temp_130557047.yaml

wind57 commented 3 years ago

I'm wondering why do parse Datree's output? If it fails, it should have exit code 1 anyway...

Right. But what is the use of that to the people using this? I mean I would like to show a human readable error message of what went wrong and where. So I wanted to use -o json, so that I could parse it with jq and extract the error message(s).

It seems you validate errors vs "suggestions" differently, which makes my code really involved. Let me explain:

  "InvalidK8sFiles": [
    {
      "Path": "/tmp/manifest.yaml",
      "ValidationErrors": [
        {
          "ErrorMessage": "could not find schema for ClusterRole"
        }
      ]
    }
  ]

Good. I can check if the output has ErrorMessage and parse the output and get the proper error message and display that to the end user.

spec:
  type: NodePort
  ports:
  - port: 8080
    targetPort: 8080
    nodePort: 30008

I get a json that contains and run : helm datree test src/main/helm -o json | jq

"EvaluationResults": {
    "FileNameRuleMapper": {
      "/tmp/manifest.yaml": {
        "7": {
          "ID": 7,
          "Name": "Prevent Service from exposing node port",
          "FailSuggestion": "Incorrect value for key `type` - `NodePort` will open a port on all nodes where it can be reached by the network external to the cluster",
          "Count": 1
        }
      }
    }

Yeah, no ErrorMessage, so my code is now supposed to check in a various place for a different thing.

imho, a simpler type like:

"EvaluationResult" : {
     "Type" : "Suggestion/Error"
     "SuggestionIfPresent" : "....."
     ....
}

So my suggestion is a unified, single place, for errors. I hope this makes sense.

@dimabru as to your suggestion, you should really maintain a "mapping"... What I mean by that: if I have 20 manifest files in a single helm chart and I display the error being in /tmp/manifest.yaml, the end user looking at those errors is going to be in the blur. Where exactly is the problem?

Thank you.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

eyarz commented 3 years ago

The output is provided by Datree CLI and not the plugin so I open a new issue there.