Better way to treat interpolation errors

[epcim]

Resolve issue #14

Add two features:

• find all missed refs. at once (before throw na error).
• same as saltclass (New pillar/master_tops saltclass module saltstack/salt#42349) can pass missed references to output (assume it's supposed to be an feature)

    -> dontpanic
        Cannot resolve ${_param:kkk}, in mykey1:path:to:fail
        Cannot resolve ${_param:kkk}, in mykey2:another:path:to:fail

[epcim]

@AndrewPickford can you have a quick look today. I would like to merge it quite soon so we move on.
Finally, it's quite simple thanks to new "settings" dict - worked in my test setup.

[AndrewPickford]

@epcim The patch should work but there's a couple of things to improve.

The catching of failed renders in Parameters._interpolate_render_value by checking for the reference sentinels at the start of the string will miss constructs that have an initial string value and then a reference.

In core._nodeinfo saving the value of self._settings.return_missing_reference and then setting it to true and then calling node.interpolate is a bit ugly. Better for node.interpolate to do the right thing based on the value of return_missing_reference.

I do like the idea of completing the parameter resolution after an error and then presenting all the errors as a list at the end. Assuming there's not too many it's nicer to show all the problems rather than one at a time.

If you can wait until the end of the week I'll try to make a few improvements and if I don't manage to get something together then we can just go ahead with the patch as is.

[epcim]

@AndrewPickford thanks for review.

1. ref items that have an initial string value - fixed, good point
2. I had the same idea. with shared _settings, I can control it cross ally entities. For now, I have just this solution. We could use deepcopy of settings, but then still node.interpolate would have to pass it to all RefItem's to override (that btw, are already created with original settings). Shortly - I tried even now - but failed

One thing to ask - https://github.com/salt-formulas/reclass/pull/17/files#diff-881f54b6c67fcdb089ddb6ccd64a4510R108 (is that right? should I use EXPORT_SENTINELS here?)

Ok, will wait until the end of the week. Just in case you have an idea.

[AndrewPickford]

@epcim Added the branch andrew-missing-ref with a modified way of producing warning messages for missing references instead of exceptions.

This adds a new method to RefItem, CompItem and ScaItem to return a plain string of the item so that a failed reference can just return the string representation of the reference. This then allows the ignore missing references logic and warning messages to be only in the parameters class. And gets rid of core._nodeinfo having to mess with the settings or check if the any references were missing.

The logic reporting failed rendering of references only raises an exception (if the ignore_missing_references setting is false) at the end of interpolation. The individual reference failure exceptions are stored as they are generated which drops the need for a pattern matching test in _interpolate_render_value.

I've also added a couple of unit tests for the new functionality.

[epcim]

Much cleaner, this is the way I wanted to implement it first. One thing still - to resolve issue #14 - in my original approach I do finally:

if not return_missing_reference:
       node.check_failed_interpolation()

I iterate over list of missed ref's and check whether they were finally interpolated or not. Remember issue #14 is about the fact that reference was not found in the given layer, but will be defined later. I have a need to report it just as a warning. Fail only if return_missing_reference=False and final dict has unresolved references.

[epcim]

@AndrewPickford

In your branch, if reference is missed, the code return's null, but I wanted to return the missed reference (in order to be able to resolve it later). This code was responsible for it:
https://github.com/salt-formulas/reclass/pull/17/files#diff-8ad6ccff91cfe56b6fdb37b619a4a525R59

We could do, sth like bewlo, on interpolate():

self._resolve_errors.resolve_errors[:] = [x for x in self._resolve_errors.resolve_errors if not re.match(self._ref_pattern, str(x.context.get_value(self._base)))]
                     
# or, equivalent
                                                                                                                           
self._resolve_errors.resolve_errors[:] = filterfalse(self._determine_reference, self._resolve_errors.resolve_errors)                                          

But first str(x.context.get_value(self._base) has to return a reference or (later updated value) instead of null.

I hope you have some clever idea now :D.

[AndrewPickford]

You're right the patch I did does not solve #14. Had a better idea for just solving this issue, by changing the ValueList.render method which handles the overwriting and catching the missing references there.

This fixes just missing references that are never used because the reference is overwritten. So as long as the last value for a scalar key is set to is either a valid reference or a scalar value then any missing references can be ignored.
An example:

# node1.tml
classes:
  - test1
  - test2
  - test3
  - test4

#test1.yml
parameters:
  a: ${x}

#test2.yml
parameters:
  a: ${y}

#test3.yml
parameters:
  a: ${z}

#test4.yml
parameters:
  z: 1

This gives:

  a: 1
  z: 1

and a message to stderr of:

[WARNING] Reference '${x}' undefined
[WARNING] Reference '${y}' undefined

This is in the branch andrew-missed-ref2.

[epcim]

I havent seen the andrew-missed-ref2 code yet, but tested it and it fails..:

➜  reclass git:(master) ✗ tree
.
├── classes
│   ├── first.yml
│   ├── second.yml
│   └── third.yml
├── nodes
│   └── dontpanic.yml
└── reclass-config.yml

2 directories, 5 files
➜  reclass git:(master) ✗ cat classes/first.yml 

parameters:
  _param:
    aaa: 111
  mykey: ${_param:aaa}
➜  reclass git:(master) ✗ cat classes/second.yml 

classes:
- first

parameters:
  _param:
     aaa: ${_param:yyy}
     ccc: ${_param:aaa}
  mykey: ${_param:ccc}
➜  reclass git:(master) ✗ cat classes/third.yml 

classes:
  - second

parameters:
  _param:
    ccc: 444
    #yyy: 555         ### HERE, keep commented, aaa: ${_param:yyy},  throw only warnings but interpolate
    kkk: 555           ### if this is commented, should print list of 3 errors
    aaa: 999
  mykey: ${_param:ccc}
  mykey2:
    tree:
      to:
        fail: ${_param:kkk}
  mkkek3:
    tree:
      to:
        fail: ${_param:kkk}
      another:
        xxxx: ${_param:kkk}

Now it again throw error on first occurrence..

Traceback (most recent call last):
  File "/home/pmichalec/hg2g/workspace-salt/reclass/reclass/values/refitem.py", line 55, in _resolve
    return path.get_value(context)
  File "/home/pmichalec/hg2g/workspace-salt/reclass/reclass/utils/dictpath.py", line 128, in get_value
    return self._get_innermost_container(base)[self._get_key()]
KeyError: 'yyy'

-> dontpanic
   Cannot resolve ${_param:yyy}, at _param:aaa, in yaml_fs:///etc/reclass/classes/second.yml

[AndrewPickford]

The andrew-missing-ref2 patch will bail out on the first error (if kkk is commented out in the example). But the fix for #14, if I've understood correctly, is not to bailout on the evaluation of aaa if yyy is missing. The grouping of multiple errors is something different.

Missed one thing in my previous comment: the setting to turn on the behaviour is now called ignore_overwritten_missing_references, which is getting over long but this is a very specific option.

I'd like to evaluate the andrew-missing-ref2 purely for fixing #14 and then add a patch for grouping multiple resolve errors together (which would be almost the same as the andrew-missing-ref patch).

[epcim]

@AndrewPickford - You are right, andrew-missingf-ref2 patch works as expected to fix #14. I missed to set the new variable and mostly found that when I was offline.

Two thing to comment, unless it affects your usage,

1. I would keep ignore_overwritten_missing_references set to True. (as it's backward compatible).
2. Update warning to more precise statement: "Reference '%s' undefined before it is first used"

It's time for patch for grouping multiple resolve errors together. That would be sweet. Let me know whether you like to merge andrew-missingf-ref2 first? Otherwise use it's branch/new for both features.

[AndrewPickford]

@epcim

Two thing to comment, unless it affects your usage,

1. I would keep ignore_overwritten_missing_references set to True. (as it's backward compatible).

Agreed, I've updated the default and pushed to andrew-missed-ref2 branch and also added some documentation.

2. Update warning to more precise statement: "Reference '%s' undefined before it is first used"

The "first used" bit is misleading. In the example for aaa: ${_param:yyy} then the error generated would be "Reference '${_param:yyy}' undefined before it is first used". But _param:yyy is never defined. It's the key aaa that is undefined when it's set to ${_param:yyy} until that is overwritten with aaa: 999. A better error message would be:

"At parameter '%s' overwritten undefined reference '%s' ignored."

But what would require the warning to be printed in the Parameters._interpolate_render_value where the name of the parameter being rendered is known. Meaning the render method would need to return a tuple of (output, warnings) which isn't a bad thing but each place calling render would need to deal with the warnings.

Instead how about for the warning:
"Undefined reference '%s' ignored during value merge"

It's time for patch for grouping multiple resolve errors together. That would be sweet. Let me know whether you like to merge andrew-missingf-ref2 first? Otherwise use it's branch/new for both features.

I've got a patch ready for grouping multiple resolve errors. Once we've sorted out the warning message I'll commit it to andrew-missed-ref2.

[epcim]

Closing in favour sophisticated approach implemented under PR #18
(let's continue with the discussion there)