Skip to content

chore: refactor JSONPatch#8497

Merged
zirain merged 2 commits intoenvoyproxy:mainfrom
zirain:refactor/josnpatch
Mar 23, 2026
Merged

chore: refactor JSONPatch#8497
zirain merged 2 commits intoenvoyproxy:mainfrom
zirain:refactor/josnpatch

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Mar 12, 2026

This patch refactor the implement of JSONPatch, prepare for making findJSONResource return multiple resources for #8489.

@zirain zirain requested a review from a team as a code owner March 12, 2026 04:58
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 12, 2026

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit 32578cf
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/69c0bb0f51b658000847ee52
😎 Deploy Preview https://deploy-preview-8497--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 65.00000% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.35%. Comparing base (dd0c09f) to head (32578cf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/jsonpatch.go 59.42% 21 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8497      +/-   ##
==========================================
+ Coverage   74.16%   74.35%   +0.18%     
==========================================
  Files         242      242              
  Lines       37804    37707      -97     
==========================================
- Hits        28039    28037       -2     
+ Misses       7809     7725      -84     
+ Partials     1956     1945      -11     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zirain zirain force-pushed the refactor/josnpatch branch from b070b99 to 8d89a83 Compare March 12, 2026 05:39
Comment thread internal/xds/translator/jsonpatch.go Outdated
@zirain zirain force-pushed the refactor/josnpatch branch from 2b8f1f8 to db29885 Compare March 13, 2026 06:42
@rudrakhp rudrakhp requested review from a team March 15, 2026 06:18
@zhaohuabing zhaohuabing requested a review from arkodg March 18, 2026 01:28
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Mar 19, 2026

do we lose any visibilty in status messages ?

  -      message: 'Validation failed for xds resource {"name":"ingress-admin","address":{"socket_address":{"address":"::","port_value":0}},...}, err:
  embedded message failed validation | caused by: value must be greater than 0.'
  +      message: 'Validation failed for xds resource type.googleapis.com/envoy.config.listener.v3.Listener, err: embedded message failed validation | cau
  sed by: value must be greater than 0.'

  -      message: 'Unable to marshal xds resource type.googleapis.com/envoy.config.listener.v3.Listener: ingress-admin, err: ...'
  +      message: 'Unable to marshal xds resource type.googleapis.com/envoy.config.listener.v3.Listener, err: ...'

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Mar 19, 2026

tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error())

I feel like it's not a good option to put the operation value into status message

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Mar 19, 2026

tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error())

I feel like it's not a good option to put the operation value into status message

ok, can we add some testcases in the json testfile to highlight the new status messages ?

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Mar 19, 2026

tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error())

I feel like it's not a good option to put the operation value into status message

ok, can we add some testcases in the json testfile to highlight the new status messages ?

There're some existing cases, and seems the refactor don't change the output.

ls internal/xds/translator/testdata/in/xds-ir | grep jsonpatch
jsonpatch-add-op-empty-jsonpath.yaml
jsonpatch-add-op-without-value.yaml
jsonpatch-invalid-listener.yaml
jsonpatch-invalid-patch.yaml
jsonpatch-invalid.yaml
jsonpatch-missing-resource.yaml
jsonpatch-move-op-with-value.yaml
jsonpatch-with-jsonpath-invalid.yaml
jsonpatch-with-jsonpath.yaml
jsonpatch.yaml

zirain added 2 commits March 23, 2026 12:01
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain force-pushed the refactor/josnpatch branch from db29885 to 32578cf Compare March 23, 2026 04:01
@zirain zirain merged commit 674f13b into envoyproxy:main Mar 23, 2026
57 of 59 checks passed
@zirain zirain deleted the refactor/josnpatch branch March 23, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants