fix: apply Easypanel config changes via redeploy - #4
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe PR adds configurable EasyPanel HTTP timeouts, service-specific configuration and redeployment flows, Compose YAML sanitization, expanded domain routing, and structured log retrieval across the protocol, server, CLI, tests, and documentation. ChangesDeployment, routing, and observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Domain mutation flowsequenceDiagram
participant CLI as domains create/remove
participant PaaS as PaaS server
participant EasyPanel as EasyPanel API
CLI->>PaaS: submit domain request
PaaS->>EasyPanel: create or delete domain
PaaS->>EasyPanel: redeploy app or Compose service
EasyPanel-->>PaaS: deployment result
PaaS-->>CLI: domain and deployment status
Structured log flowsequenceDiagram
participant Client as GetLogs client
participant PaaS as PaaS server
participant EasyPanel as EasyPanel log route
Client->>PaaS: request service logs and limit
PaaS->>EasyPanel: query app or Compose logs
EasyPanel-->>PaaS: Loki-shaped log response
PaaS-->>Client: LogEntry records and flat text
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
316-317: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new Compose rejections and the RemoveDomain behavior.
sanitizeComposenow does more than stripports:. It rejects content without a top-levelservicesmapping, rejects a service entry that is not a mapping, and rejectsnetwork_mode: host, all withInvalidArgument. Add these rules here. Also update theRemoveDomainrow at line 185, because that RPC now redeploys routing and can returnAbortedafter a successful deletion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 316 - 317, Update the README’s Compose Port Security section to document that sanitizeCompose rejects missing top-level services, non-mapping service entries, and network_mode: host with InvalidArgument, in addition to stripping ports. Revise the RemoveDomain table entry to state that it redeploys routing and may return Aborted after a successful deletion.
🧹 Nitpick comments (3)
internal/server/paas.go (1)
768-774: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the unsupported-type helper.
Lines 768-771 repeat the pattern of
requireDeployConfigSupport. Only the route name and the remedy text differ. Extend the helper to take the missing route name, then call it here so all unsupported-compose errors stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/paas.go` around lines 768 - 774, Update requireDeployConfigSupport to accept the missing route name and use it to construct the unsupported-service error while preserving the existing remedy text. Replace the inline compose check in the surrounding handler with a call to requireDeployConfigSupport for the compose resource-update route, keeping the existing resources validation unchanged.cmd/domains.go (1)
135-144: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a delay between the two redeploy attempts.
The loop retries immediately after a failure, so a transient panel error is likely to repeat.
RemoveDomainininternal/server/paas.gowaits one second between attempts. Match that behavior.♻️ Proposed change
for attempt := 0; attempt < 2; attempt++ { deployErr = redeployAfterConfig(c, args[0], args[1], svcType) if deployErr == nil { break } + if attempt == 0 { + time.Sleep(time.Second) + } }Add
timeto the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/domains.go` around lines 135 - 144, Update the retry loop around redeployAfterConfig in the domain removal flow to wait one second before the second attempt, matching RemoveDomain’s retry behavior. Add the required time dependency and preserve the existing two-attempt limit and error handling.compose_test.go (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip instead of falling back to a hardcoded panel domain.
Line 33 returns a specific panel host when
DEFAULT_DOMAINis unset. The test then sends requests to that third-party panel.paas_test.goalready solves this withdefaultPanelDomainOrSkip. Reuse the skip behavior here.♻️ Proposed change
-func panelDomain() string { - if v := os.Getenv("DEFAULT_DOMAIN"); v != "" { - return v - } - return "cv911b.easypanel.host" -} +func panelDomain(t *testing.T) string { + t.Helper() + v := os.Getenv("DEFAULT_DOMAIN") + if v == "" { + t.Skip("DEFAULT_DOMAIN not set") + } + return v +}Update the two call sites at lines 194 and 347 to pass
t.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compose_test.go` around lines 29 - 34, Update panelDomain and its call sites to use the existing defaultPanelDomainOrSkip behavior from paas_test.go instead of returning the hardcoded cv911b.easypanel.host fallback. Pass t at both callers around the existing panel-domain usages so tests skip when DEFAULT_DOMAIN is unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/domains.go`:
- Around line 74-94: Update the destination construction in the domains create
flow to use the parsed path variable for destination.path instead of a hardcoded
root path. Validate svcType before invoking redeployAfterConfig, accepting only
app or compose and rejecting unknown --type values; preserve composeService’s
existing behavior of forcing compose.
In `@cmd/services.go`:
- Around line 304-308: Update the command flow around redeployAfterConfig so the
success message distinguishes whether deployment actually ran: retain “Env
updated and deployed” for app and compose services, and print an update-only
message for other service types where redeployAfterConfig performs no
deployment. Use the service type already available from args[2] and keep the
existing error handling unchanged.
In `@compose_test.go`:
- Around line 54-59: Update the poll loop around client.Get to build and execute
an HTTP request using the test context, satisfying noctx, and handle
request-construction or execution errors through the existing lastErr flow.
Before closing the successful response body, drain it with io.Copy and handle
any resulting errors so errcheck is satisfied and connections can be reused; add
the required context and io imports.
In `@internal/server/paas.go`:
- Around line 743-753: Update ScaleService to fetch the current inspect deploy
object before calling services.%s.updateDeploy, then merge only req.Replicas
into that object while preserving existing deploy settings such as zeroDowntime
and command. Pass the merged deploy payload to s.ep.Call, keeping the existing
deploy-config validation and scaling route intact.
---
Outside diff comments:
In `@README.md`:
- Around line 316-317: Update the README’s Compose Port Security section to
document that sanitizeCompose rejects missing top-level services, non-mapping
service entries, and network_mode: host with InvalidArgument, in addition to
stripping ports. Revise the RemoveDomain table entry to state that it redeploys
routing and may return Aborted after a successful deletion.
---
Nitpick comments:
In `@cmd/domains.go`:
- Around line 135-144: Update the retry loop around redeployAfterConfig in the
domain removal flow to wait one second before the second attempt, matching
RemoveDomain’s retry behavior. Add the required time dependency and preserve the
existing two-attempt limit and error handling.
In `@compose_test.go`:
- Around line 29-34: Update panelDomain and its call sites to use the existing
defaultPanelDomainOrSkip behavior from paas_test.go instead of returning the
hardcoded cv911b.easypanel.host fallback. Pass t at both callers around the
existing panel-domain usages so tests skip when DEFAULT_DOMAIN is unset.
In `@internal/server/paas.go`:
- Around line 768-774: Update requireDeployConfigSupport to accept the missing
route name and use it to construct the unsupported-service error while
preserving the existing remedy text. Replace the inline compose check in the
surrounding handler with a call to requireDeployConfigSupport for the compose
resource-update route, keeping the existing resources validation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4df99f93-ee6c-4326-9eef-d698d08c0cb7
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumproto/paas.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
.env.exampleREADME.mdcmd/domains.gocmd/panel_client.gocmd/services.gocmd/services_test.gocompose_test.gogo.modgrpc_e2e_test.gointernal/easypanel/client.gointernal/easypanel/client_test.gointernal/server/compose_test.gointernal/server/logs.gointernal/server/logs_test.gointernal/server/paas.gopaas_test.goproto/paas.proto
| resp, err := client.Get(url) | ||
| if err != nil { | ||
| lastErr = err | ||
| } else { | ||
| last = resp.StatusCode | ||
| resp.Body.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Satisfy the noctx and errcheck linters in the poll loop.
golangci-lint flags client.Get at line 54 and the unchecked resp.Body.Close at line 59. Build a request with the test context and drain the body before closing. Draining also allows connection reuse across poll attempts.
🔧 Proposed change
for time.Now().Before(deadline) {
- resp, err := client.Get(url)
+ req, reqErr := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil)
+ if reqErr != nil {
+ t.Fatalf("build request for %s: %v", url, reqErr)
+ }
+ resp, err := client.Do(req)
if err != nil {
lastErr = err
} else {
last = resp.StatusCode
- resp.Body.Close()
+ _, _ = io.Copy(io.Discard, resp.Body)
+ _ = resp.Body.Close()
if resp.StatusCode < 400 {
return resp.StatusCode
}
}Add context and io to the imports.
📝 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.
| resp, err := client.Get(url) | |
| if err != nil { | |
| lastErr = err | |
| } else { | |
| last = resp.StatusCode | |
| resp.Body.Close() | |
| req, reqErr := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) | |
| if reqErr != nil { | |
| t.Fatalf("build request for %s: %v", url, reqErr) | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| lastErr = err | |
| } else { | |
| last = resp.StatusCode | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| _ = resp.Body.Close() | |
| if resp.StatusCode < 400 { | |
| return resp.StatusCode | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 59-59: Error return value of resp.Body.Close is not checked
(errcheck)
[error] 54-54: (*net/http.Client).Get must not be called. use (*net/http.Client).Do(*http.Request)
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose_test.go` around lines 54 - 59, Update the poll loop around client.Get
to build and execute an HTTP request using the test context, satisfying noctx,
and handle request-construction or execution errors through the existing lastErr
flow. Before closing the successful response body, drain it with io.Copy and
handle any resulting errors so errcheck is satisfied and connections can be
reused; add the required context and io imports.
Source: Linters/SAST tools
Root causes: - updateSourceInline expects `content`, not `composeContent` - Easypanel persists env/source/domain config but regenerates runtime/Traefik overrides only on deploy; domains created after initial compose deploy stayed 502 - compose test published host ports owned by Traefik - panel deploy calls can exceed old 30/60s client timeouts Changes: - automatically deploy app/compose after env, source, build, deploy/resource, and domain mutations; compose creation deploys once for service discovery and once after domains - resolve domain destination before deletion, preserve legacy ID-only RPC, validate stale caller context, retry routing deploy, and report partial success - fix CLI payload schema: content/ref/path/rootPath/wildcard/nested deploy - replace text ports stripping with structural YAML sanitization; reject host network - use real logs.queryServiceLogs/queryComposeServiceLogs routes and parse Loki entries without timestamp precision loss - raise configurable panel timeout to 5m and surface nested zod field errors - extend GetLogs/Domain protobuf messages and regenerate Go bindings - replace sleep-only/false-positive E2E checks with routed 2xx/3xx polling Verified against live panel: WordPress compose HTTP 200 after post-domain redeploy; gRPC app+compose deployment, resource/deploy updates, legacy RemoveDomain all pass.
336dd7d to
37ab25e
Compare
1 participant
Root cause
Two separate failures formed one config-application bug:
services.compose.updateSourceInlineexpectscontent; CLI/test sentcomposeContent, causing400 ... zodErrors.content: Required.502indefinitely while WordPress/Apache was healthy.Minimal live proof:
Fix
domain_id-only clients, validates stale caller context, retries deploy once, supports idempotent partial-success recovery, and returns explicit manual-deploy guidance if application fails.EASYPANEL_HTTP_TIMEOUT(default5m); reusable client raised from 60s. Compose pulls can block for minutes.Other panel API drift fixed during schema audit
ref+ requiredpathinstead of unsupportedbranchrootPathwildcard; documented port/HTTPS/compose flags now implementeddeployobject, not top-levelreplicaslogs.queryServiceLogs/queryComposeServiceLogs; Loki-shaped response parsed into flat text + structured protobuf entries without float timestamp lossSecurity hardening
Old line-prefix
ports:sanitizer could be bypassed by quoted keys or inline mappings. New structural YAML sanitizer removes short/long-form/quoted/mergedports, preservesexpose, rejects invalid shapes, and rejectsnetwork_mode: host.Protobuf additions (backward wire-compatible)
GetLogsRequest: service type, compose service, limitGetLogsResponse: structuredLogEntrylist while retaining flatlogsTests / evidence
Fresh local verification:
Live Easypanel E2E:
Added regression tests for structural Compose sanitization, host networking, YAML merge-key ports, invalid shapes, nil resources, legacy/compose domain target resolution, nanosecond log ordering, Loki flattening, and nested zod errors.
Final independent code review: no P0/P1 release blockers.
Summary by CodeRabbit