Skip to content

Conversation

zirain
Copy link
Member

@zirain zirain commented Sep 19, 2025

What type of PR is this?

xref: #6752

@zirain zirain requested a review from a team as a code owner September 19, 2025 05:03
@zirain zirain changed the title translator: move EnvoyPorxy validation out of Provider translator: move EnvoyProxy validation out of Provider Sep 19, 2025
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 75.24752% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.03%. Comparing base (58eab94) to head (8569d11).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/provider/kubernetes/controller.go 47.36% 10 Missing ⚠️
internal/gatewayapi/translator.go 88.57% 6 Missing and 2 partials ⚠️
internal/gatewayapi/runner/runner.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7009      +/-   ##
==========================================
- Coverage   71.05%   71.03%   -0.03%     
==========================================
  Files         227      227              
  Lines       40441    40495      +54     
==========================================
+ Hits        28736    28765      +29     
- Misses      10009    10028      +19     
- Partials     1696     1702       +6     

☔ 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.

rudrakhp
rudrakhp previously approved these changes Sep 19, 2025
Copy link
Member

@rudrakhp rudrakhp left a comment

Choose a reason for hiding this comment

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

LGTM with one question

}
case egv1a1.ProxyAccessLogFormatTypeJSON:
if setting.Format.JSON == nil {
err := fmt.Errorf("unable to configure access log when using JSON format but \"json\" field being empty")
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this check is removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

EG use JSON as default now, and nil means use default JSON output.

cnvergence
cnvergence previously approved these changes Sep 19, 2025
Copy link
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

👍🏻

@zirain zirain dismissed stale reviews from cnvergence and rudrakhp via 9ace30c September 20, 2025 03:01
rudrakhp
rudrakhp previously approved these changes Sep 20, 2025
cnvergence
cnvergence previously approved these changes Sep 24, 2025
@zirain zirain dismissed stale reviews from cnvergence and rudrakhp via a59d4c9 September 24, 2025 23:15
@zirain zirain force-pushed the fix-6752 branch 2 times, most recently from f40b7ec to adcc255 Compare September 26, 2025 05:29
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>

r.log.Error(err, fmt.Sprintf("failed process TLS SecretRef for EnvoyProxy for gatewayClass %s", managedGC.Name))
gc := status.SetGatewayClassAccepted(
managedGC.DeepCopy(),
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't understand why we do deepcopy here, we post GC with old status to gatewayapi layer in the past.

cc @arkodg

Signed-off-by: zirain <[email protected]>
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