✨ Add Proxy support to rukpak generator #1998
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1998 +/- ##
==========================================
+ Coverage 69.11% 69.14% +0.02%
==========================================
Files 79 79
Lines 7023 7062 +39
==========================================
+ Hits 4854 4883 +29
- Misses 1887 1896 +9
- Partials 282 283 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -56,11 +56,16 @@ func (r ResourceGenerators) ResourceGenerator() ResourceGenerator { | |||
|
|
|||
| type UniqueNameGenerator func(string, interface{}) (string, error) | |||
|
|
|||
| type Proxy struct { | |||
There was a problem hiding this comment.
nit: should we call it ProxyConfig or something?
| } | ||
| } | ||
|
|
||
| func addProxyEnvVars(httpProxy, httpsProxy, noProxy string, containers []corev1.Container) { |
There was a problem hiding this comment.
should we nuke any preexisting proxy env vars?
| @@ -115,6 +115,40 @@ func WithMutatingWebhooks(webhooks ...admissionregistrationv1.MutatingWebhook) f | |||
| } | |||
| } | |||
|
|
|||
| // With | |||
| cs := containers | ||
| for i := range cs { |
There was a problem hiding this comment.
Is this needed? Can't we just do
| cs := containers | |
| for i := range cs { | |
| for container, _ := range containers { |
| func addProxyEnvVars(httpProxy, httpsProxy, noProxy string, containers []corev1.Container) { | ||
| cs := containers | ||
| for i := range cs { | ||
| if len(httpProxy) > 0 { |
There was a problem hiding this comment.
nit
| if len(httpProxy) > 0 { | |
| if len(httpProxy) != 0 { |
seems cleaner
There was a problem hiding this comment.
Code Review
I've reviewed the proxy support implementation. Here are my findings:
**File: **
Line 119 - Missing documentation:
The function needs better documentation. Consider adding:
Line 122 - Limited scope:
This only handles Deployment objects. Consider expanding to support DaemonSet, StatefulSet, and Job workloads for completeness.
Line 128 - Missing validation:
Consider adding input validation for proxy URLs and checking for existing environment variables to avoid duplicates.
**File: **
Line 282 - Test coverage:
Good test! Consider adding edge cases like empty proxy values.
Overall, the implementation follows good patterns but could be more robust.
| @@ -115,6 +115,40 @@ func WithMutatingWebhooks(webhooks ...admissionregistrationv1.MutatingWebhook) f | |||
| } | |||
| } | |||
|
|
|||
| // With | |||
| func WithProxy(httpProxy, httpsProxy, noProxy string) func(client.Object) { | |||
There was a problem hiding this comment.
Missing documentation for this function. Consider adding proper JSDoc-style comments explaining the parameters and behavior.
There was a problem hiding this comment.
(this was generated by Claude -- just picked on this PR to try out automated reviews XD)
Description
Reviewer Checklist