Skip to content

[LIB] Fix the behavior of null-preserving visitor, refactor, add comparison tests - #4774

Open
mozesl-nokia wants to merge 12 commits into
kptdev:mainfrom
nokia:fix-preserve-null
Open

mozesl-nokia wants to merge 12 commits into
kptdev:mainfrom
nokia:fix-preserve-null

Conversation

@mozesl-nokia

@mozesl-nokia mozesl-nokia commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

#4746 didn't cover enough of the use-cases for the null preservation behaviour. This PR refactors a big chunk of that code, splitting the visitor into it's own class and reverted the changes on the old custom Visitor (except for the associative list bugfix).

Added some comparison tests so we can track how each of our visitors handle merges compared to each other and kyaml's original.

AI disclosure:

  • Cursor Grok 4.7 was used to complete NullPreservingVisitor, add the comparison tests and for review

Update

  • Renamed PreserveExplicitNull to PreserveNulls
    • Had to uncomment the api replace, plan to fix it immediately after
    • Added an "Experimental" marker to it, surprisingly we have already used this kind of marking for the --for-deployment flag of kpt pkg get
  • Fixed schema generation
    • It has not been run at least since we migrated the api into it's own module, thus some very big diff there.
    • It should now be running when you do make generate

AI disclosure:

  • Later used Grok 4.7 to update the docs and relevant docstrings after renaming things.

@mozesl-nokia
mozesl-nokia requested review from a team September 25, 2026 15:49
@mozesl-nokia mozesl-nokia added bug Something isn't working cleanup area/fn-runtime KRM function runtime go Pull requests that update Go code labels Sep 25, 2026
@netlify

netlify Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for kptdocs ready!

Name Link
🔨 Latest commit 6c84812
🔍 Latest deploy log https://app.netlify.com/projects/kptdocs/deploys/6abe2141c678cf0008067dcf
😎 Deploy Preview https://deploy-preview-4774--kptdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@mozesl-nokia
mozesl-nokia marked this pull request as ready for review September 29, 2026 15:35
@mozesl-nokia
mozesl-nokia requested review from a team and a balanced review from Copilot September 29, 2026 15:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Upstream-null associative lists are not reliably preserved, and the expanded behavior conflicts with existing public documentation.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Refactors merge3 null preservation into a dedicated visitor and expands behavioral comparisons.

Changes:

  • Separates default and null-preserving merge visitors.
  • Adds shared null-handling utilities and associative-list preprocessing.
  • Replaces legacy tests with broader preservation and kyaml comparison coverage.
File Description
visitor.go Restores the default merge visitor behavior.
visitor_util.go Adds shared null-handling helpers.
tuple.go Selects the appropriate visitor and preprocesses null lists.
preserve_test.go Removes superseded preservation tests.
null_preserving_visitor.go Implements dedicated null-preserving merge behavior.
null_preserve_test.go Tests scalar and associative-list null preservation.
comparison_test.go Compares custom visitors with kyaml behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/lib/update/merge3/null_preserving_visitor.go
@mozesl-nokia

mozesl-nokia commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Also, the "PreserveExplicitNull" is now misleading, because this implementation preserves all null values, not just the explicit ones. How awkward would it be to break fresh api changes by renaming it? @kptdev/maintainers

Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
@mozesl-nokia
mozesl-nokia requested a balanced review from Copilot September 30, 2026 15:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The default associative-list merge has a null-deletion regression, and schema generation is not reliably pinned.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 2 Low severity

Open (5)
Resolved since last review (1)

Comment thread pkg/lib/update/merge3/visitor.go Outdated
Comment thread Makefile Outdated
Comment thread scripts/generate-schema.sh Outdated
Comment thread documentation/content/en/reference/schema/kptfile/kptfile.json Outdated
Comment thread documentation/content/en/reference/schema/kptfile/kptfile.yaml Outdated
Assisted-by: Cursor:grok-4.7
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Legacy Kptfile compatibility and associative-list null-preservation issues remain unresolved.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (5)

Comment thread api/kptfile/v1/types.go
@sonarqubecloud

sonarqubecloud Bot commented Oct 1, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
11.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@mozesl-nokia
mozesl-nokia requested a review from a team October 2, 2026 11:00

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/fn-runtime KRM function runtime bug Something isn't working cleanup go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants