Fail generation and add help on field typos

JustinKuli commented 2 years ago

When the input configuration does not match the schema in the go type, the generator will now respond with an error. The intention is to help users when there is a typo in the configuration, which otherwise would just be ignored (and in the case of a map, any configuration "inside" would be dropped).

This also adds help text to these error cases, to try and identify what field the user might have meant. That implementation uses a library which does not compute the Levenshtein distance, but should work well enough, and has the advantage of already being in the import graph.

Refs:

https://github.com/stolostron/backlog/issues/19646

Signed-off-by: Justin Kulikauskas jkulikau@redhat.com

openshift-ci[bot] commented 2 years ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

JustinKuli commented 2 years ago

Just a draft for now, I want to add a test, but I want to make sure the direction I'm taking is not the wrong one.

With this change, running the generator directly with this config in the examples directory:

apiVersion: policy.open-cluster-management.io/v1
kind: PolicyGenerator
metadata:
  name: policy-generator-name
placementBindingDefaults:
  name: my-placement-binding
policyDefaults:
  controls: 
    - PR.DS-1 Data-at-rest
  namespace: my-policies
  placement:
    placementRulePath: input/placementrule.yaml
  remediationAction: inform
  severity: medium
  policySet: 
    - policyset-kyverno
policies:
- name: policy-app-config-aliens
  disabled: false
  manifests:
    - path: input/configmap-aliens.yaml
      patches:
        - apiVersion: v1
          kind: ConfigMap
          metadata:
            labels:
              chandler: bing
  remediationAction: enforce
policySets:
- name: policyset-kyverno
  description: this is a kyverno policy set.
  policies: []
  placement:
    placementRulePath: input/placementrule.yaml

Produces this error:

error processing the PolicyGenerator file 'policyGenerator.yaml': the PolicyGenerator configuration file is invalid: yaml: unmarshal errors:
  line 15: field policySet not found in type types.PolicyDefaults - did you mean 'policySets'?

mprahl commented 2 years ago

Just a draft for now, I want to add a test, but I want to make sure the direction I'm taking is not the wrong one.

With this change, running the generator directly with this config in the examples directory:

apiVersion: policy.open-cluster-management.io/v1
kind: PolicyGenerator
metadata:
  name: policy-generator-name
placementBindingDefaults:
  name: my-placement-binding
policyDefaults:
  controls: 
    - PR.DS-1 Data-at-rest
  namespace: my-policies
  placement:
    placementRulePath: input/placementrule.yaml
  remediationAction: inform
  severity: medium
  policySet: 
    - policyset-kyverno
policies:
- name: policy-app-config-aliens
  disabled: false
  manifests:
    - path: input/configmap-aliens.yaml
      patches:
        - apiVersion: v1
          kind: ConfigMap
          metadata:
            labels:
              chandler: bing
  remediationAction: enforce
policySets:
- name: policyset-kyverno
  description: this is a kyverno policy set.
  policies: []
  placement:
    placementRulePath: input/placementrule.yaml

Produces this error:

error processing the PolicyGenerator file 'policyGenerator.yaml': the PolicyGenerator configuration file is invalid: yaml: unmarshal errors:
  line 15: field policySet not found in type types.PolicyDefaults - did you mean 'policySets'?

This is cool. The issue is that types.PolicyDefaults means nothing to the user since that is a Go construct. You could add logic so that it converts types.PolicyDefaults to policyDefaults.

mprahl commented 2 years ago

@JustinKuli I think we could simplify this to just point to the path in the YAML that is invalid rather than supply a suggestion. What do you think?

JustinKuli commented 2 years ago

Partly I added the suggestion to make the change more than just 3 lines 😉 ... but I also do think it is pretty helpful. Especially in this example case, where the user is just missing the s, without a suggestion I might look back and forth between my YAML and the example multiple times and never see the difference.

You could add logic so that it converts types.PolicyDefaults to policyDefaults.

That could be good. While we're at it, I would consider rephrasing more of the error, so it's more like:

line 15: field policySet found but not defined in type policyDefaults - did you mean 'policySets'?

What do you think?

mprahl commented 2 years ago

Partly I added the suggestion to make the change more than just 3 lines wink ... but I also do think it is pretty helpful. Especially in this example case, where the user is just missing the s, without a suggestion I might look back and forth between my YAML and the example multiple times and never see the difference.

You could add logic so that it converts types.PolicyDefaults to policyDefaults.

That could be good. While we're at it, I would consider rephrasing more of the error, so it's more like:
line 15: field policySet found but not defined in type policyDefaults - did you mean 'policySets'?
What do you think?

That sounds good.

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, JustinKuli, mprahl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/stolostron/policy-generator-plugin/blob/main/OWNERS)~~ [JustinKuli,dhaiducek,mprahl] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment