Add merge.Override transform

[kanterov]

Changes

Add merge.Override transform. It allows the override one dyn.Value with another, preserving source locations for parts of the sub-tree where nothing has changed. This is different from merging, where values are concatenated.

OverrideVisitor is visiting the changes during the override process and allows to control of what changes are allowed or update the effective value.

The primary use case is Python code updating bundle configuration.

During override, we update locations only for changed values. This allows us to keep track of locations where values were initially defined and used for error reporting. For instance, merging:

resources:               # location=left.yaml:0
  jobs:                  # location=left.yaml:1
    job_0:               # location=left.yaml:2
      name: "job_0"      # location=left.yaml:3

with

resources:               # location=right.yaml:0
  jobs:                  # location=right.yaml:1
    job_0:               # location=right.yaml:2
      name: "job_0"      # location=right.yaml:3
      description: job 0 # location=right.yaml:4
    job_1:               # location=right.yaml:5
      name: "job_1"      # location=right.yaml:5

produces

resources:               # location=left.yaml:0
  jobs:                  # location=left.yaml:1
    job_0:               # location=left.yaml:2
      name: "job_0"      # location=left.yaml:3
      description: job 0 # location=right.yaml:4
    job_1:               # location=right.yaml:5
      name: "job_1"      # location=right.yaml:5

Tests

Unit tests

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 75.26882% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 55.10%. Comparing base (e22dd8a) to head (2ca1c25).
Report is 106 commits behind head on main.

Files	Patch %	Lines
libs/dyn/merge/override.go	75.26%	13 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
+ Coverage   52.25%   55.10%   +2.84%     
==========================================
  Files         317      346      +29     
  Lines       18004    15785    -2219     
==========================================
- Hits         9408     8698     -710     
+ Misses       7903     6294    -1609     
- Partials      693      793     +100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pietern]

This is generic functionality so should belong somewhere in libs/dyn. It could be part of the merge package perhaps?

[pietern]

This hook is executed only for the top level value, not for inner values. Is this intentional?

[kanterov]

Yes, because we want to handle top-level value from inner values differently. E.g., appending to top-level is allowed during pre-init, but appending to nested field is not allowed, because it will mutate existing resource.

[kanterov]

We can allow to "undelete" value here, but there is no use-case for it right now.

[pietern]

Looks great!

Haven't reviewed the tests yet, doing that later today.

[pietern]

Missing call to the update visitor?

[kanterov]

This is very intentional, I can add a comment. We don't know if the value was changed or not. That's why we call "update" only on leafs

[pietern]

Very nice!

Please update the PR description (and title perhaps) before merging.

[kanterov]

@pietern Thanks. I've updated the code, PR description, and title. Can you add it to the merge queue?

[pietern]

Yes, everything looks good!

Looking at the code coverage, could you add/modify tests to make sure we cover the error cases as well? I think the table test can be modified to run twice for every example, one like it does now, and another one where the visitor returns an error to confirm it actually bubbles up to the caller.