Add the Lease Proxy Server implementation#4961
Add the Lease Proxy Server implementation#4961openshift-merge-bot[bot] merged 7 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTracks an HTTP server address into Config, exposes it as LEASE_PROXY_SERVER_URL, introduces a lease proxy HTTP component that forwards acquire/release requests to a pluggable lease client, and extends lease client/fake APIs (configurable randID and predefined resources); steps and tests updated to wire mux and address. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Proxy as Lease Proxy Handler
participant LeaseClient as Lease Client
participant LeaseServer as Lease Backend
Client->>Proxy: POST /lease/acquire?type=TYPE&count=N
activate Proxy
Proxy->>Proxy: validate params
Proxy->>LeaseClient: Acquire(type, count)
activate LeaseClient
LeaseClient->>LeaseServer: Acquire request
LeaseServer-->>LeaseClient: lease names
deactivate LeaseClient
Proxy-->>Client: 200 OK { "names": [...] }
deactivate Proxy
Client->>Proxy: POST /lease/release?name=NAME
activate Proxy
Proxy->>Proxy: validate params
Proxy->>LeaseClient: Release(name)
activate LeaseClient
LeaseClient->>LeaseServer: Release request
LeaseServer-->>LeaseClient: OK
deactivate LeaseClient
Proxy-->>Client: 204 No Content
deactivate Proxy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/lease/proxy/proxy.go`:
- Around line 42-45: The validation error message incorrectly references
parameter "n"; update the error text to reference "count" instead: in the
strconv.ParseUint call that assigns c, err (the block using
strconv.ParseUint(count, 10, strconv.IntSize)), replace fmt.Errorf("parameter
\"n\" is not valid: %s", count) with an error message that names "count" (e.g.,
fmt.Errorf("parameter \"count\" is not valid: %s", count)) so callers see the
correct query parameter in proxy.go.
- Around line 106-120: The Content-Type header is being set after writing the
response body so it won't be sent; move w.Header().Set("Content-Type",
"application/json") to before the first write to the ResponseWriter (i.e.,
before the call that writes namesBytes via w.Write in the handler), ensuring the
header is set prior to sending the AcquireResponse JSON payload (refer to
AcquireResponse, namesBytes and the w.Write call); keep error handling paths
as-is but ensure any successful JSON response sets the header before writing.
In `@pkg/steps/lease_proxy.go`:
- Around line 49-59: The run() implementation on stepLeaseProxyServer
dereferences s.leaseClient and uses s.srvMux without nil checks which can panic;
add validation in the stepLeaseProxyServer.Validate() method to return a clear
error when s.leaseClient is nil or s.srvMux is nil (and optionally validate
s.logger), and update run() (the helper run(context.Context) and Run(ctx
context.Context) flow) to avoid dereferencing when those fields are nil (e.g.,
return an error from run if validation failed) so leaseproxy.New and
proxy.RegisterHandlers are only called when s.leaseClient and s.srvMux are
non-nil.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (15)
cmd/ci-operator/main.gopkg/api/constant.gopkg/defaults/config.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/lease/client_test.gopkg/lease/fake.gopkg/lease/proxy/proxy.gopkg/lease/proxy/proxy_test.gopkg/steps/ip_pool_test.gopkg/steps/lease_proxy.gopkg/steps/lease_proxy_test.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
| func (*stepLeaseProxyServer) Objects() []ctrlruntimeclient.Object { return nil } | ||
| func (*stepLeaseProxyServer) Validate() error { return nil } | ||
|
|
||
| func (s *stepLeaseProxyServer) Run(ctx context.Context) error { | ||
| return results.ForReason("executing_lease_proxy").ForError(s.run(ctx)) | ||
| } | ||
|
|
||
| //nolint:unparam // Remove this as soon as this functions can return an error as well. | ||
| func (s *stepLeaseProxyServer) run(context.Context) error { | ||
| if 1 == 2 { | ||
| return fmt.Errorf("unreachable code to make the linter happy. Will be removed soon.") | ||
| } | ||
| s.logger.Info("TODO - Not implemented") | ||
| proxy := leaseproxy.New(s.logger, func() lease.Client { return *s.leaseClient }) | ||
| proxy.RegisterHandlers(s.srvMux) |
There was a problem hiding this comment.
Add nil checks for leaseClient and srvMux to prevent panic.
run() dereferences s.leaseClient and uses s.srvMux without validation. A miswired config would panic at runtime. Consider validating these in Validate().
🛠️ Proposed fix
import (
"context"
+ "errors"
"net/http"
"github.com/sirupsen/logrus"
@@
-func (*stepLeaseProxyServer) Validate() error { return nil }
+func (s *stepLeaseProxyServer) Validate() error {
+ if s.leaseClient == nil {
+ return errors.New("lease proxy server requires a lease client")
+ }
+ if s.srvMux == nil {
+ return errors.New("lease proxy server requires an HTTP server mux")
+ }
+ return nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (*stepLeaseProxyServer) Objects() []ctrlruntimeclient.Object { return nil } | |
| func (*stepLeaseProxyServer) Validate() error { return nil } | |
| func (s *stepLeaseProxyServer) Run(ctx context.Context) error { | |
| return results.ForReason("executing_lease_proxy").ForError(s.run(ctx)) | |
| } | |
| //nolint:unparam // Remove this as soon as this functions can return an error as well. | |
| func (s *stepLeaseProxyServer) run(context.Context) error { | |
| if 1 == 2 { | |
| return fmt.Errorf("unreachable code to make the linter happy. Will be removed soon.") | |
| } | |
| s.logger.Info("TODO - Not implemented") | |
| proxy := leaseproxy.New(s.logger, func() lease.Client { return *s.leaseClient }) | |
| proxy.RegisterHandlers(s.srvMux) | |
| func (*stepLeaseProxyServer) Objects() []ctrlruntimeclient.Object { return nil } | |
| func (s *stepLeaseProxyServer) Validate() error { | |
| if s.leaseClient == nil { | |
| return errors.New("lease proxy server requires a lease client") | |
| } | |
| if s.srvMux == nil { | |
| return errors.New("lease proxy server requires an HTTP server mux") | |
| } | |
| return nil | |
| } | |
| func (s *stepLeaseProxyServer) Run(ctx context.Context) error { | |
| return results.ForReason("executing_lease_proxy").ForError(s.run(ctx)) | |
| } | |
| //nolint:unparam // Remove this as soon as this functions can return an error as well. | |
| func (s *stepLeaseProxyServer) run(context.Context) error { | |
| proxy := leaseproxy.New(s.logger, func() lease.Client { return *s.leaseClient }) | |
| proxy.RegisterHandlers(s.srvMux) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/steps/lease_proxy.go` around lines 49 - 59, The run() implementation on
stepLeaseProxyServer dereferences s.leaseClient and uses s.srvMux without nil
checks which can panic; add validation in the stepLeaseProxyServer.Validate()
method to return a clear error when s.leaseClient is nil or s.srvMux is nil (and
optionally validate s.logger), and update run() (the helper run(context.Context)
and Run(ctx context.Context) flow) to avoid dereferencing when those fields are
nil (e.g., return an error from run if validation failed) so leaseproxy.New and
proxy.RegisterHandlers are only called when s.leaseClient and s.srvMux are
non-nil.
There was a problem hiding this comment.
Nice catch for srvMux but the validation logic won't work for leaseClient. The lease client gets initialized after the validation procedure executes. If the initialization fails, the step isn't executed at all, therefore it should be quite safe to use it in this way.
There was a problem hiding this comment.
@danilo-gemoli, thank you for the clarification! You're absolutely right—if leaseClient is initialized after validation and the step won't run on initialization failure, then the nil check for it isn't necessary.
I'll revise the suggestion to only validate srvMux:
🛠️ Revised suggestion
import (
"context"
+ "errors"
"net/http"
"github.com/sirupsen/logrus"
@@
-func (*stepLeaseProxyServer) Validate() error { return nil }
+func (s *stepLeaseProxyServer) Validate() error {
+ if s.srvMux == nil {
+ return errors.New("lease proxy server requires an HTTP server mux")
+ }
+ return nil
+}📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
/test e2e |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/lease/client.go (1)
49-51: Guard against a nil randID override.A nil
randIDwill panic at call sites. Keeping the default when nil makes the option safer.♻️ Suggested tweak
func WithRandID(randID func() string) ClientOptions { - return func(o *clientOptions) { o.randID = randID } + return func(o *clientOptions) { + if randID != nil { + o.randID = randID + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lease/client.go` around lines 49 - 51, The WithRandID option should guard against a nil randID to avoid panics; update the WithRandID function so it checks if randID != nil before assigning to clientOptions.randID (i.e., only override o.randID when the provided randID is non-nil), leaving the existing default in clientOptions unchanged when nil is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/lease/fake.go`:
- Around line 47-60: The fake client currently discards any error returned by
addCall when a predefined resource is found; in AcquireWaitWithPriority and
Acquire return the found resource together with err instead of returning (res,
nil). Update the success branches in fakeClient.AcquireWaitWithPriority (lookup
key "acquireWaitWithPriority_%s_%s_%s_%s") and fakeClient.Acquire (lookup key
"acquire_%s_%s_%s") to return (res, err) so injected failures from addCall are
preserved.
In `@pkg/lease/proxy/proxy.go`:
- Around line 38-46: Parsed "count" currently allows zero; change the validation
so that after parsing (the strconv.ParseUint branch) you check that c > 0 and
return an error if c == 0 (e.g., fmt.Errorf("parameter \"count\" must be > 0:
%d", c)). Assign params.count only when c > 0; make the error surface as a
client/bad-request response (HTTP 400) so callers and Acquire (which expects
n>0) do not receive an empty-request.
In `@pkg/steps/lease_proxy.go`:
- Around line 52-57: The Validate method on stepLeaseProxyServer currently only
checks srvMux; also verify that the server address is set by checking s.srvAddr
(the field used to build LEASE_PROXY_SERVER_URL in Provides()) and return an
error if it's empty so downstream code doesn't get an empty
LEASE_PROXY_SERVER_URL; update stepLeaseProxyServer.Validate() to return a
descriptive error when s.srvAddr == "" (referencing Validate(),
stepLeaseProxyServer, srvAddr, and Provides()).
---
Nitpick comments:
In `@pkg/lease/client.go`:
- Around line 49-51: The WithRandID option should guard against a nil randID to
avoid panics; update the WithRandID function so it checks if randID != nil
before assigning to clientOptions.randID (i.e., only override o.randID when the
provided randID is non-nil), leaving the existing default in clientOptions
unchanged when nil is passed.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/lease/client.gopkg/lease/fake.gopkg/lease/proxy/proxy.gopkg/steps/lease_proxy.go
| func (c *fakeClient) AcquireWaitWithPriority(ctx context.Context, rtype, state, dest, requestID string) (*common.Resource, error) { | ||
| err := c.addCall("acquireWaitWithPriority", rtype, state, dest, requestID) | ||
| if res, ok := c.resources[fmt.Sprintf("acquireWaitWithPriority_%s_%s_%s_%s", rtype, state, dest, requestID)]; ok { | ||
| return res, nil | ||
| } | ||
| return &common.Resource{Name: fmt.Sprintf("%s_%d", rtype, len(*c.calls)-1)}, err | ||
| } | ||
|
|
||
| func (c *fakeClient) Acquire(rtype, state, dest string) (*common.Resource, error) { | ||
| err := c.addCall("acquire", rtype, state, dest) | ||
| if res, ok := c.resources[fmt.Sprintf("acquire_%s_%s_%s", rtype, state, dest)]; ok { | ||
| return res, nil | ||
| } | ||
| return &common.Resource{Name: fmt.Sprintf("%s_%d", rtype, len(*c.calls)-1)}, err |
There was a problem hiding this comment.
Preserve injected failures even when a predefined resource exists.
addCall can return an error, but the current branch returns nil when a resource is found, masking failures in tests. Return the resource with err instead.
🛠️ Suggested fix
if res, ok := c.resources[fmt.Sprintf("acquireWaitWithPriority_%s_%s_%s_%s", rtype, state, dest, requestID)]; ok {
- return res, nil
+ return res, err
}
@@
if res, ok := c.resources[fmt.Sprintf("acquire_%s_%s_%s", rtype, state, dest)]; ok {
- return res, nil
+ return res, err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/lease/fake.go` around lines 47 - 60, The fake client currently discards
any error returned by addCall when a predefined resource is found; in
AcquireWaitWithPriority and Acquire return the found resource together with err
instead of returning (res, nil). Update the success branches in
fakeClient.AcquireWaitWithPriority (lookup key
"acquireWaitWithPriority_%s_%s_%s_%s") and fakeClient.Acquire (lookup key
"acquire_%s_%s_%s") to return (res, err) so injected failures from addCall are
preserved.
There was a problem hiding this comment.
I don't want to address this issue in this PR
There was a problem hiding this comment.
@danilo-gemoli, understood! Would you like me to open a GitHub issue to track this for a future PR?
| count := r.URL.Query().Get("count") | ||
| if count == "" { | ||
| params.count = 1 | ||
| } else { | ||
| c, err := strconv.ParseUint(count, 10, strconv.IntSize) | ||
| if err != nil { | ||
| return params, fmt.Errorf("parameter \"count\" is not valid: %s", count) | ||
| } | ||
| params.count = uint(c) |
There was a problem hiding this comment.
Reject count=0 to align with lease client expectations.
Acquire expects n > 0. Accepting count=0 returns an empty lease list, which is likely a caller error. Consider returning 400 instead.
🛠️ Suggested fix
} else {
c, err := strconv.ParseUint(count, 10, strconv.IntSize)
if err != nil {
return params, fmt.Errorf("parameter \"count\" is not valid: %s", count)
}
+ if c == 0 {
+ return params, errors.New("count must be greater than 0")
+ }
params.count = uint(c)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| count := r.URL.Query().Get("count") | |
| if count == "" { | |
| params.count = 1 | |
| } else { | |
| c, err := strconv.ParseUint(count, 10, strconv.IntSize) | |
| if err != nil { | |
| return params, fmt.Errorf("parameter \"count\" is not valid: %s", count) | |
| } | |
| params.count = uint(c) | |
| count := r.URL.Query().Get("count") | |
| if count == "" { | |
| params.count = 1 | |
| } else { | |
| c, err := strconv.ParseUint(count, 10, strconv.IntSize) | |
| if err != nil { | |
| return params, fmt.Errorf("parameter \"count\" is not valid: %s", count) | |
| } | |
| if c == 0 { | |
| return params, errors.New("count must be greater than 0") | |
| } | |
| params.count = uint(c) | |
| } |
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 41-41: : # github.com/openshift/ci-tools/pkg/api/testimagestreamtagimport/v1 [github.com/openshift/ci-tools/pkg/api/testimagestreamtagimport/v1.test]
pkg/api/testimagestreamtagimport/v1/register.go:41:3: cannot use &TestImageStreamTagImport{} (value of type *TestImageStreamTagImport) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to scheme.AddKnownTypes: *TestImageStreamTagImport does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
pkg/api/testimagestreamtagimport/v1/register.go:42:3: cannot use &TestImageStreamTagImportList{} (value of type *TestImageStreamTagImportList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to scheme.AddKnownTypes: *TestImageStreamTagImportList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/lease/proxy/proxy.go` around lines 38 - 46, Parsed "count" currently
allows zero; change the validation so that after parsing (the strconv.ParseUint
branch) you check that c > 0 and return an error if c == 0 (e.g.,
fmt.Errorf("parameter \"count\" must be > 0: %d", c)). Assign params.count only
when c > 0; make the error surface as a client/bad-request response (HTTP 400)
so callers and Acquire (which expects n>0) do not receive an empty-request.
danilo-gemoli
force-pushed
the
feat/ci-operator/lease-proxy-server
branch
from
b019180 to
69a6390
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/defaults/defaults.go (1)
1244-1255:⚠️ Potential issue | 🟠 MajorGuard lease proxy step when HTTP server address/mux are missing.
isLeaseProxyServerAvailableonly checksLeaseClientEnabled, so a missing HTTP server addr/mux can still create a lease proxy step that failsValidate(). Gate on the HTTP server inputs to avoid failing runs when the HTTP server isn’t configured.🛠️ Suggested fix
func isLeaseProxyServerAvailable(cfg *Config) bool { - return cfg.LeaseClientEnabled + return cfg.LeaseClientEnabled && cfg.HTTPServerAddr != "" && cfg.HTTPServerMux != nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults.go` around lines 1244 - 1255, isLeaseProxyServerAvailable currently only checks cfg.LeaseClientEnabled which allows leaseProxyServerStep to build a step even when HTTP inputs are missing; update isLeaseProxyServerAvailable to also verify cfg.HTTPServerAddr is non-empty and cfg.HTTPServerMux is non-nil (and optionally cfg.LeaseClient is non-nil) so leaseProxyServerStep returns an empty slice when any required HTTP server input is missing, preventing a step that would fail Validate(); adjust references in leaseProxyServerStep to rely on the updated isLeaseProxyServerAvailable guard.
♻️ Duplicate comments (2)
pkg/lease/fake.go (1)
47-51:⚠️ Potential issue | 🟡 MinorReturn injected errors alongside predefined resources.
addCallcan inject failures; returning(res, nil)masks them and makes tests unable to assert error paths.🛠️ Suggested fix
if res, ok := c.resources[fmt.Sprintf("acquireWaitWithPriority_%s_%s_%s_%s", rtype, state, dest, requestID)]; ok { - return res, nil + return res, err } @@ if res, ok := c.resources[fmt.Sprintf("acquire_%s_%s_%s", rtype, state, dest)]; ok { - return res, nil + return res, err }Also applies to: 56-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lease/fake.go` around lines 47 - 51, The test double masks injected errors from addCall: in fakeClient.AcquireWaitWithPriority (and the analogous method around lines 56-59) you should check the error returned by c.addCall(...) and, if non-nil, return that error instead of returning (res, nil); locate the calls to addCall in the methods named AcquireWaitWithPriority (and the other fakeClient method using the resources map) and change the control flow to return (nil, err) when addCall returns an error, otherwise proceed to look up and return the resource from c.resources as before.pkg/lease/proxy/proxy.go (1)
38-89:⚠️ Potential issue | 🟡 MinorReject count=0 instead of returning an empty lease list.
Acquireexpectsn > 0; returning 200 forcount=0hides caller errors.🛠️ Suggested fix
c, err := strconv.ParseUint(count, 10, strconv.IntSize) if err != nil { return params, fmt.Errorf("parameter \"count\" is not valid: %s", count) } + if c == 0 { + return params, errors.New("count must be greater than 0") + } params.count = uint(c)- if params.count == 0 { - p.writeAcquireResponse(w, []string{}) - return - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lease/proxy/proxy.go` around lines 38 - 89, The handler Proxy.acquire currently accepts count==0 and returns an empty lease list; instead, detect when params.count == 0 after parseAcquireParams and respond with a 400 Bad Request (log via p.logger.WithField/WithError or Warnf) and an explanatory error message (e.g. "count must be > 0") rather than calling p.writeAcquireResponse; update the logic in Proxy.acquire to reject zero counts and return immediately so callers receive an error instead of a 200 with an empty list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/defaults/defaults.go`:
- Around line 1244-1255: isLeaseProxyServerAvailable currently only checks
cfg.LeaseClientEnabled which allows leaseProxyServerStep to build a step even
when HTTP inputs are missing; update isLeaseProxyServerAvailable to also verify
cfg.HTTPServerAddr is non-empty and cfg.HTTPServerMux is non-nil (and optionally
cfg.LeaseClient is non-nil) so leaseProxyServerStep returns an empty slice when
any required HTTP server input is missing, preventing a step that would fail
Validate(); adjust references in leaseProxyServerStep to rely on the updated
isLeaseProxyServerAvailable guard.
---
Duplicate comments:
In `@pkg/lease/fake.go`:
- Around line 47-51: The test double masks injected errors from addCall: in
fakeClient.AcquireWaitWithPriority (and the analogous method around lines 56-59)
you should check the error returned by c.addCall(...) and, if non-nil, return
that error instead of returning (res, nil); locate the calls to addCall in the
methods named AcquireWaitWithPriority (and the other fakeClient method using the
resources map) and change the control flow to return (nil, err) when addCall
returns an error, otherwise proceed to look up and return the resource from
c.resources as before.
In `@pkg/lease/proxy/proxy.go`:
- Around line 38-89: The handler Proxy.acquire currently accepts count==0 and
returns an empty lease list; instead, detect when params.count == 0 after
parseAcquireParams and respond with a 400 Bad Request (log via
p.logger.WithField/WithError or Warnf) and an explanatory error message (e.g.
"count must be > 0") rather than calling p.writeAcquireResponse; update the
logic in Proxy.acquire to reject zero counts and return immediately so callers
receive an error instead of a 200 with an empty list.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (16)
cmd/ci-operator/main.gopkg/api/constant.gopkg/defaults/config.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/lease/client.gopkg/lease/client_test.gopkg/lease/fake.gopkg/lease/proxy/proxy.gopkg/lease/proxy/proxy_test.gopkg/steps/ip_pool_test.gopkg/steps/lease_proxy.gopkg/steps/lease_proxy_test.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/steps/ip_pool_test.go
- pkg/defaults/config.go
- pkg/api/constant.go
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/steps/multi_stage/multi_stage.go
- pkg/steps/lease_test.go
- pkg/lease/client_test.go
|
/test e2e |
|
/test e2e |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/defaults/defaults.go (1)
124-127: Loop variableleaseProxyServerStepshadows the function of the same name.The iteration variable and the function it ranges over share the identical identifier. While Go evaluates the
rangeexpression before entering the loop body so there is no runtime ambiguity here, this name collision makes the code harder to scan and is a trap for future refactors.♻️ Proposed rename of loop variable
- for _, leaseProxyServerStep := range leaseProxyServerStep(cfg) { - buildSteps = append(buildSteps, leaseProxyServerStep) - addProvidesForStep(leaseProxyServerStep, cfg.params) + for _, lps := range leaseProxyServerStep(cfg) { + buildSteps = append(buildSteps, lps) + addProvidesForStep(lps, cfg.params) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults.go` around lines 124 - 127, The loop variable leaseProxyServerStep shadows the function leaseProxyServerStep; rename the iteration variable (e.g., to step or leaseProxyStep) in the for-range to avoid the name collision and update its usages inside the loop (buildSteps = append(buildSteps, <newName>) and addProvidesForStep(<newName>, cfg.params)) so the code references the new loop variable while leaving the leaseProxyServerStep function unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/defaults/defaults.go`:
- Around line 1244-1246: Update the guard in isLeaseProxyServerAvailable to also
verify cfg.LeaseClient != nil; currently it only checks cfg.LeaseClientEnabled,
cfg.HTTPServerAddr, and cfg.HTTPServerMux, so add the nil-check to avoid a
potential panic in stepLeaseProxyServer.run() when the anonymous function
dereferences LeaseClient—modify the function isLeaseProxyServerAvailable to
return cfg.LeaseClientEnabled && cfg.LeaseClient != nil && cfg.HTTPServerAddr !=
"" && cfg.HTTPServerMux != nil.
---
Nitpick comments:
In `@pkg/defaults/defaults.go`:
- Around line 124-127: The loop variable leaseProxyServerStep shadows the
function leaseProxyServerStep; rename the iteration variable (e.g., to step or
leaseProxyStep) in the for-range to avoid the name collision and update its
usages inside the loop (buildSteps = append(buildSteps, <newName>) and
addProvidesForStep(<newName>, cfg.params)) so the code references the new loop
variable while leaving the leaseProxyServerStep function unchanged.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/defaults/defaults.go
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
/test e2e |
|
/test e2e |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/defaults/defaults_test.go (1)
1866-1868: Use the API constant for the env var key to avoid drift.At Line 1867, prefer
api.LeaseProxyServerURLEnvVarNameinstead of the raw"LEASE_PROXY_SERVER_URL"string so this test stays aligned with the public constant.♻️ Proposed change
expectedParams: map[string]string{ - "LEASE_PROXY_SERVER_URL": "http://10.0.0.1:8080", + api.LeaseProxyServerURLEnvVarName: "http://10.0.0.1:8080", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 1866 - 1868, Replace the hard-coded env var key in the test's expectedParams map with the public API constant to avoid drift: change the map key currently using the literal "LEASE_PROXY_SERVER_URL" to use api.LeaseProxyServerURLEnvVarName (the test referencing expectedParams in defaults_test.go) so the test stays aligned with the constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/defaults/defaults_test.go`:
- Around line 1866-1868: Replace the hard-coded env var key in the test's
expectedParams map with the public API constant to avoid drift: change the map
key currently using the literal "LEASE_PROXY_SERVER_URL" to use
api.LeaseProxyServerURLEnvVarName (the test referencing expectedParams in
defaults_test.go) so the test stays aligned with the constant.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/defaults/defaults_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test images |
|
@danilo-gemoli: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit 9baa38a
into
openshift:main
2 participants
#4958 introduced an HTTP server in
ci-operator, while this PR adds some route to it:implemented by
pkg/lease/proxy/proxy.go.The proxy server is quite dumb as it just relies on the lease client to forward the requests it gets from a multi-stage step. It does nothing more than that so far.
The
stepLeaseProxyServerexposes theLEASE_PROXY_SERVER_URLenvironment variable that, in turns, is consumed bymultiStageTestStep.A multi-stage step pod uses
$LEASE_PROXY_SERVER_URLto make calls like this:$ curl "${LEASE_PROXY_SERVER_URL}/lease/acquire?type=aws-quota-slice"Summary by CodeRabbit
New Features
Tests