feat: add service exec and resolve IDs from service#173
Merged
Conversation
… of context Add `zeabur service exec` to run commands inside service containers via the `executeCommand` GraphQL mutation. Remove context-based ID resolution from all commands. When a service ID is provided, environment and project IDs are now derived from the service itself rather than relying on the (often wrong) project context. This eliminates mismatches where context points to a different project. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThe PR systematically removes context-based parameter defaulting from command initialization across ~25 command files, replacing PreRunE hooks with explicit API-based ID resolution during execution. A new service exec command is added, along with supporting API methods and model types for command execution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
internal/cmd/variable/delete/delete.go (2)
134-154:⚠️ Potential issue | 🟡 MinorDisplay messages may show empty service name when
--idis used.Same issue as other variable commands -
opts.namewill be empty if--idis provided directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/delete/delete.go` around lines 134 - 154, The displayed service name can be empty when the command was invoked with --id; update all user-facing messages to fall back to the identifier when opts.name is empty (e.g. use a local variable like displayName := opts.name; if displayName == "" { displayName = opts.id }) and use displayName in spinner.WithSuffix, the fmt.Errorf call, and f.Log.Infof so messages never show an empty service name; make no behavioral changes to UpdateVariables or error handling.
132-152:⚠️ Potential issue | 🟠 MajorBug: Spinner is created but never started.
The spinner
sis created at line 132 buts.Start()is never called. The subsequents.Stop()calls at lines 145, 149, and 152 will attempt to stop an unstarted spinner, which is a no-op but indicates missing functionality.Suggested fix
s := spinner.New(cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, spinner.WithColor(cmdutil.SpinnerColor), spinner.WithSuffix(fmt.Sprintf(" Deleting variables of service: %s...", opts.name)), ) + s.Start() for k, v := range opts.keys {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/delete/delete.go` around lines 132 - 152, The spinner `s` is created but never started; call s.Start() immediately after creating the spinner (right after spinner.New) and then defer s.Stop() so the spinner is stopped on all exit paths; update the block around the spinner creation (variable `s`) in the delete flow, start the spinner before calling f.ApiClient.UpdateVariables (which updates opts.keys via UpdateVariables) and either remove the explicit s.Stop() calls in the error branches or keep them (defer ensures a final stop) to avoid leaving the spinner unstarted or leaking runtime state for service `opts.name`.internal/cmd/variable/update/update.go (2)
130-143:⚠️ Potential issue | 🟠 MajorBug: Spinner is created but never started.
The spinner
sis created at line 130 buts.Start()is never called. Thes.Stop()calls at lines 136, 140, and 143 will operate on an unstarted spinner.Suggested fix
s := spinner.New(cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, spinner.WithColor(cmdutil.SpinnerColor), spinner.WithSuffix(fmt.Sprintf(" Updating variables of service: %s...", opts.name)), ) + s.Start() + createVarResult, err := f.ApiClient.UpdateVariables(context.Background(), opts.id, opts.environmentID, opts.keys)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/update/update.go` around lines 130 - 143, Spinner `s` is constructed but never started; call s.Start() immediately after creating it and ensure it is stopped on all exit paths (preferably with defer s.Stop() right after starting) around the call to f.ApiClient.UpdateVariables in function handling opts (use the same spinner variable `s`), so the spinner runs while UpdateVariables executes and is reliably stopped whether UpdateVariables returns an error, false, or success.
43-51:⚠️ Potential issue | 🔴 CriticalBug:
opts.keysis reset, discarding any values passed via--keyflag.Line 44 reinitializes
opts.keysto an empty map, which discards any key-value pairs passed via the--keyflag before entering either the interactive or non-interactive flow. Non-interactive mode with--keyflags will not work as expected.Suggested fix
func runUpdateVariable(f *cmdutil.Factory, opts *Options) error { - opts.keys = make(map[string]string) - if f.Interactive { + opts.keys = make(map[string]string) return runUpdateVariableInteractive(f, opts) } else { return runUpdateVariableNonInteractive(f, opts) } }Only initialize
opts.keysin the interactive path where it's needed for the prompt flow. In non-interactive mode,opts.keysshould retain the values passed via--key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/update/update.go` around lines 43 - 51, The code currently resets opts.keys at the start of runUpdateVariable, discarding any --key flag values; remove that global reinitialization and instead initialize opts.keys only when entering the interactive flow: in runUpdateVariable, check f.Interactive and call runUpdateVariableInteractive(f, opts) (ensure that function initializes opts.keys = make(map[string]string) before prompting), while leaving opts.keys untouched before calling runUpdateVariableNonInteractive so supplied --key flags are preserved; update any related logic in runUpdateVariableInteractive and runUpdateVariableNonInteractive as needed to rely on opts.keys being present or initialized there.internal/cmd/variable/list/list.go (1)
85-101:⚠️ Potential issue | 🟡 MinorSpinner and log messages may show empty service name when
--idis used.Similar to other variable commands, when
--idis provided directly without--name, the spinner suffix (line 87) and log messages (lines 97, 101, 106) will display an empty string.Consider computing
idOrNamefor display purposes as done inredeploy.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/list/list.go` around lines 85 - 101, Spinner suffix and subsequent log messages use opts.name which can be empty when the user supplies --id; compute a display string (e.g., idOrName) that prefers opts.name but falls back to opts.id and use that in spinner.WithSuffix and in f.Log.Infof calls (replace occurrences referencing opts.name in this file's ListVariables flow with the new idOrName variable); mirror the idOrName construction pattern used in redeploy.go so all UX messages show the id when name is missing.internal/cmd/variable/create/create.go (1)
109-112:⚠️ Potential issue | 🟡 MinorSpinner and log messages may show empty service name.
When
--idis provided directly without--name,opts.nameremains empty. The spinner suffix (line 111), error message (line 135), and success message (line 139) will display an empty string for the service name.Consider using the same pattern as seen in
redeploy.go(lines 86-89) whereidOrNameis computed:Suggested fix
+ // to show friendly message + idOrName := opts.name + if idOrName == "" { + idOrName = opts.id + } + s := spinner.New(cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, spinner.WithColor(cmdutil.SpinnerColor), - spinner.WithSuffix(fmt.Sprintf(" Creating variables of service: %s...", opts.name)), + spinner.WithSuffix(fmt.Sprintf(" Creating variables of service: %s...", idOrName)), )Then use
idOrNamein subsequent messages as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/create/create.go` around lines 109 - 112, The spinner and log messages use opts.name which can be empty when the user supplied --id; compute an idOrName fallback (e.g., idOrName := opts.name; if idOrName == "" { idOrName = opts.id }) as done in redeploy.go, then replace usages of opts.name in spinner.WithSuffix, the error message in the create flow, and the success message with idOrName so the UI always shows the provided identifier; update references around the spinner variable s and the fmt.Sprintf calls that build those messages accordingly.internal/cmd/service/restart/restart.go (1)
91-100:⚠️ Potential issue | 🟡 MinorIncorrect confirmation message: says "deploy" instead of "restart".
The confirmation prompt text is incorrect for this command.
Proposed fix
- confirm, err := f.Prompter.Confirm(fmt.Sprintf("Are you sure to deploy service <%s>?", idOrName), true) + confirm, err := f.Prompter.Confirm(fmt.Sprintf("Are you sure to restart service <%s>?", idOrName), true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/service/restart/restart.go` around lines 91 - 100, The confirmation prompt text is wrong — update the message passed to f.Prompter.Confirm so it asks about restarting rather than deploying; specifically change the fmt.Sprintf(...) call that currently uses "Are you sure to deploy service <%s>?" to reference "restart" and keep the same idOrName variable and current behavior around f.Interactive and opts.skipConfirm (the call to f.Prompter.Confirm, the confirm/error handling, and returned results should remain unchanged).
🧹 Nitpick comments (2)
internal/cmd/service/network/network.go (1)
39-63: Fallback display name when only--idis provided.If users pass only
--id,opts.namestays empty, so the spinner/log output reads with a blank service name. A small fallback improves UX without extra API calls.💡 Suggested tweak
+ displayName := opts.name + if displayName == "" { + displayName = opts.id + } + s := spinner.New(cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, spinner.WithColor(cmdutil.SpinnerColor), - spinner.WithSuffix(fmt.Sprintf(" Fetching network information of service %s ...", opts.name)), + spinner.WithSuffix(fmt.Sprintf(" Fetching network information of service %s ...", displayName)), ) @@ - f.Log.Infof("Private DNS name for %s: %s", opts.name, dnsName+".zeabur.internal") + f.Log.Infof("Private DNS name for %s: %s", displayName, dnsName+".zeabur.internal")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/service/network/network.go` around lines 39 - 63, When only --id is provided runNetwork leaves opts.name empty causing blank labels; set a fallback so opts.name is populated when opts.id is present (e.g., if opts.name == "" && opts.id != "" then set opts.name = opts.id) before any spinner/log output or later use; update the logic in runNetwork (around uses of opts.id and opts.name) so the display always shows a name without making extra API calls.internal/cmd/domain/delete/delete.go (1)
109-123: Avoid unnecessary environment resolution in non-interactive delete.
RemoveDomainonly uses the domain name, so resolvingenvironmentIDhere adds an extra API call and a potential failure path without any functional benefit. Consider dropping this block unless you plan to useenvironmentIDlater in this function.♻️ Suggested simplification
- if opts.environmentID == "" && opts.id != "" { - envID, err := util.ResolveEnvironmentIDByServiceID(f.ApiClient, opts.id) - if err != nil { - return err - } - opts.environmentID = envID - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/domain/delete/delete.go` around lines 109 - 123, The code is making an unnecessary API call to resolve environmentID via util.ResolveEnvironmentIDByServiceID when deleting a domain even though RemoveDomain only needs the domain name; remove the block that checks opts.environmentID == "" and calls ResolveEnvironmentIDByServiceID so you don't call the API or introduce a failure path, leaving only the service name → ID resolution (util.GetServiceByName) and then call RemoveDomain with opts.name (and opts.id if still needed); ensure no other code in this function relies on opts.environmentID before deleting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmd/service/expose/expose.go`:
- Around line 65-85: The code can pass an empty projectID into ExposeService
when --env-id is provided because service.Project is not validated; add a guard
so we fail fast: after fetching service (in the same block that currently sets
opts.environmentID via util.ResolveEnvironmentID) check if opts.environmentID !=
"" (or simply check when opts.environmentID was provided) and if service.Project
== nil or service.Project.ID == "" return a clear error like "service has no
associated project" before computing projectID or calling ExposeService; ensure
the same validation logic used for the ResolveEnvironmentID branch is applied so
projectID is never left as an empty string.
In `@internal/cmd/service/metric/metric.go`:
- Around line 75-116: The MetricType is constructed from raw opts.metricType so
lowercase flag values bypass the WithMeasureUnit switch; change the construction
to use the normalized value by replacing mt := model.MetricType(opts.metricType)
with mt := model.MetricType(upperCaseMetricType) (and optionally validate
allowed values) so WithMeasureUnit and any switches on MetricType (referencing
mt and the constants "CPU","MEMORY","NETWORK") will match correctly.
In `@internal/cmd/template/deploy/deploy.go`:
- Around line 93-102: Add an explicit check for the case when both opts.region
and opts.projectID are provided and surface a warning (or validation) instead of
silently ignoring region: before the existing if block that creates a project,
add logic that if opts.region != "" && opts.projectID != "" then either (a) log
a warning via f.Log.Warnf that the --region flag will be ignored because
--project-id was provided, or (b) perform validation by calling
f.ApiClient.GetProject(ctx, opts.projectID) and compare the returned
project.Region to opts.region and return an error if they differ; reference
opts.region, opts.projectID, f.Log and f.ApiClient.GetProject/GetProjectByID (or
CreateProject) when implementing.
---
Outside diff comments:
In `@internal/cmd/service/restart/restart.go`:
- Around line 91-100: The confirmation prompt text is wrong — update the message
passed to f.Prompter.Confirm so it asks about restarting rather than deploying;
specifically change the fmt.Sprintf(...) call that currently uses "Are you sure
to deploy service <%s>?" to reference "restart" and keep the same idOrName
variable and current behavior around f.Interactive and opts.skipConfirm (the
call to f.Prompter.Confirm, the confirm/error handling, and returned results
should remain unchanged).
In `@internal/cmd/variable/create/create.go`:
- Around line 109-112: The spinner and log messages use opts.name which can be
empty when the user supplied --id; compute an idOrName fallback (e.g., idOrName
:= opts.name; if idOrName == "" { idOrName = opts.id }) as done in redeploy.go,
then replace usages of opts.name in spinner.WithSuffix, the error message in the
create flow, and the success message with idOrName so the UI always shows the
provided identifier; update references around the spinner variable s and the
fmt.Sprintf calls that build those messages accordingly.
In `@internal/cmd/variable/delete/delete.go`:
- Around line 134-154: The displayed service name can be empty when the command
was invoked with --id; update all user-facing messages to fall back to the
identifier when opts.name is empty (e.g. use a local variable like displayName
:= opts.name; if displayName == "" { displayName = opts.id }) and use
displayName in spinner.WithSuffix, the fmt.Errorf call, and f.Log.Infof so
messages never show an empty service name; make no behavioral changes to
UpdateVariables or error handling.
- Around line 132-152: The spinner `s` is created but never started; call
s.Start() immediately after creating the spinner (right after spinner.New) and
then defer s.Stop() so the spinner is stopped on all exit paths; update the
block around the spinner creation (variable `s`) in the delete flow, start the
spinner before calling f.ApiClient.UpdateVariables (which updates opts.keys via
UpdateVariables) and either remove the explicit s.Stop() calls in the error
branches or keep them (defer ensures a final stop) to avoid leaving the spinner
unstarted or leaking runtime state for service `opts.name`.
In `@internal/cmd/variable/list/list.go`:
- Around line 85-101: Spinner suffix and subsequent log messages use opts.name
which can be empty when the user supplies --id; compute a display string (e.g.,
idOrName) that prefers opts.name but falls back to opts.id and use that in
spinner.WithSuffix and in f.Log.Infof calls (replace occurrences referencing
opts.name in this file's ListVariables flow with the new idOrName variable);
mirror the idOrName construction pattern used in redeploy.go so all UX messages
show the id when name is missing.
In `@internal/cmd/variable/update/update.go`:
- Around line 130-143: Spinner `s` is constructed but never started; call
s.Start() immediately after creating it and ensure it is stopped on all exit
paths (preferably with defer s.Stop() right after starting) around the call to
f.ApiClient.UpdateVariables in function handling opts (use the same spinner
variable `s`), so the spinner runs while UpdateVariables executes and is
reliably stopped whether UpdateVariables returns an error, false, or success.
- Around line 43-51: The code currently resets opts.keys at the start of
runUpdateVariable, discarding any --key flag values; remove that global
reinitialization and instead initialize opts.keys only when entering the
interactive flow: in runUpdateVariable, check f.Interactive and call
runUpdateVariableInteractive(f, opts) (ensure that function initializes
opts.keys = make(map[string]string) before prompting), while leaving opts.keys
untouched before calling runUpdateVariableNonInteractive so supplied --key flags
are preserved; update any related logic in runUpdateVariableInteractive and
runUpdateVariableNonInteractive as needed to rely on opts.keys being present or
initialized there.
---
Nitpick comments:
In `@internal/cmd/domain/delete/delete.go`:
- Around line 109-123: The code is making an unnecessary API call to resolve
environmentID via util.ResolveEnvironmentIDByServiceID when deleting a domain
even though RemoveDomain only needs the domain name; remove the block that
checks opts.environmentID == "" and calls ResolveEnvironmentIDByServiceID so you
don't call the API or introduce a failure path, leaving only the service name →
ID resolution (util.GetServiceByName) and then call RemoveDomain with opts.name
(and opts.id if still needed); ensure no other code in this function relies on
opts.environmentID before deleting.
In `@internal/cmd/service/network/network.go`:
- Around line 39-63: When only --id is provided runNetwork leaves opts.name
empty causing blank labels; set a fallback so opts.name is populated when
opts.id is present (e.g., if opts.name == "" && opts.id != "" then set opts.name
= opts.id) before any spinner/log output or later use; update the logic in
runNetwork (around uses of opts.id and opts.name) so the display always shows a
name without making extra API calls.
Comment on lines
+65
to
85
| // Resolve environment and project from the service | ||
| service, err := f.ApiClient.GetService(context.Background(), opts.id, "", "", "") | ||
| if err != nil { | ||
| return fmt.Errorf("get service failed: %w", err) | ||
| } | ||
|
|
||
| if opts.environmentID == "" { | ||
| projectID := f.Config.GetContext().GetProject().GetID() | ||
| envID, err := util.ResolveEnvironmentID(f.ApiClient, projectID) | ||
| if service.Project == nil || service.Project.ID == "" { | ||
| return fmt.Errorf("service has no associated project") | ||
| } | ||
| envID, err := util.ResolveEnvironmentID(f.ApiClient, service.Project.ID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| opts.environmentID = envID | ||
| } | ||
|
|
||
| err := paramCheck(opts) | ||
| if err != nil { | ||
| return err | ||
| projectID := "" | ||
| if service.Project != nil { | ||
| projectID = service.Project.ID | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ExposeService usage and signature
rg -n --context 4 "ExposeService" internal/cmd -A 5 -B 2Repository: zeabur/cli
Length of output: 677
🏁 Script executed:
#!/bin/bash
# Also search for where ExposeService might be defined in the pkg directory
fd -e go -type f | xargs rg -l "ExposeService" | head -20Repository: zeabur/cli
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Check the specific file to understand the context around the snippet
cat -n internal/cmd/service/expose/expose.go | head -100Repository: zeabur/cli
Length of output: 3389
🏁 Script executed:
#!/bin/bash
# Find ExposeService function definition
rg -n "func.*ExposeService" pkg/api/service.go -A 15Repository: zeabur/cli
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Also check the interface definition
rg -n "ExposeService" pkg/api/interface.go -B 2 -A 2Repository: zeabur/cli
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Find ObjectID implementation
rg -n "func.*ObjectID" pkg/api -A 5Repository: zeabur/cli
Length of output: 336
🏁 Script executed:
#!/bin/bash
# Also look for the pattern where projectID is used elsewhere to understand expectations
rg -n "projectID" pkg/api/service.go | head -20Repository: zeabur/cli
Length of output: 2137
Fail fast if the service has no associated project before calling ExposeService.
When --env-id is provided, the code skips the Project validation at line 72 but still derives projectID from the service. If service.Project is nil, projectID becomes an empty string, causing the GraphQL call to fail with a less clear error. Add an explicit check to match the validation pattern used when environment resolution is needed.
Suggested guard
+ if service.Project == nil || service.Project.ID == "" {
+ return fmt.Errorf("service has no associated project")
+ }
projectID := service.Project.ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/service/expose/expose.go` around lines 65 - 85, The code can
pass an empty projectID into ExposeService when --env-id is provided because
service.Project is not validated; add a guard so we fail fast: after fetching
service (in the same block that currently sets opts.environmentID via
util.ResolveEnvironmentID) check if opts.environmentID != "" (or simply check
when opts.environmentID was provided) and if service.Project == nil or
service.Project.ID == "" return a clear error like "service has no associated
project" before computing projectID or calling ExposeService; ensure the same
validation logic used for the ResolveEnvironmentID branch is applied so
projectID is never left as an empty string.
Comment on lines
+75
to
+116
| if opts.id == "" && opts.name != "" { | ||
| service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| opts.environmentID = envID | ||
| opts.id = service.ID | ||
| } | ||
|
|
||
| if err := paramCheck(opts); err != nil { | ||
| return err | ||
| if opts.id == "" { | ||
| return fmt.Errorf("--id or --name is required") | ||
| } | ||
|
|
||
| // if name is set, get service id by name | ||
| if opts.id == "" && opts.name != "" { | ||
| service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) | ||
| // Resolve environment and project from the service | ||
| service, err := f.ApiClient.GetService(context.Background(), opts.id, "", "", "") | ||
| if err != nil { | ||
| return fmt.Errorf("get service failed: %w", err) | ||
| } | ||
|
|
||
| projectID := "" | ||
| if service.Project != nil { | ||
| projectID = service.Project.ID | ||
| } | ||
|
|
||
| if opts.environmentID == "" { | ||
| envID, err := util.ResolveEnvironmentID(f.ApiClient, projectID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| opts.id = service.ID | ||
| opts.environmentID = envID | ||
| } | ||
|
|
||
| upperCaseMetricType := strings.ToUpper(opts.metricType) | ||
| if opts.metricType == "" { | ||
| return fmt.Errorf("metric type is required") | ||
| } | ||
|
|
||
| upperCaseMetricType := strings.ToUpper(opts.metricType) | ||
| mt := model.MetricType(opts.metricType) | ||
|
|
||
| startTime := time.Now().Add(-time.Duration(opts.hour) * time.Hour) | ||
| endTime := time.Now() | ||
|
|
||
| metrics, err := f.ApiClient.ServiceMetric(context.Background(), opts.id, f.Config.GetContext().GetProject().GetID(), opts.environmentID, upperCaseMetricType, startTime, endTime) | ||
| metrics, err := f.ApiClient.ServiceMetric(context.Background(), opts.id, projectID, opts.environmentID, upperCaseMetricType, startTime, endTime) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the metric.go file
find . -name "metric.go" -path "*/internal/cmd/service/metric/*" | head -5Repository: zeabur/cli
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Search for MetricType definition
rg -n "type MetricType" --type goRepository: zeabur/cli
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Search for WithMeasureUnit usage
rg -n "WithMeasureUnit" --type go -B 2 -A 2Repository: zeabur/cli
Length of output: 1071
🏁 Script executed:
#!/bin/bash
# Search for ValidArgs in metric context
rg -n "ValidArgs" internal/cmd/service/metric/ --type go -B 2 -A 2Repository: zeabur/cli
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Get MetricType constant definitions
rg -n "MetricType[A-Z]" pkg/model/service.go -A 1Repository: zeabur/cli
Length of output: 391
🏁 Script executed:
#!/bin/bash
# Get full WithMeasureUnit implementation
rg -n "func \(m MetricType\) WithMeasureUnit" -A 20 --type goRepository: zeabur/cli
Length of output: 1034
🏁 Script executed:
#!/bin/bash
# Get the full context around ValidArgs in metric.go
sed -n '30,40p' internal/cmd/service/metric/metric.goRepository: zeabur/cli
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Verify the exact code around lines 75-116 in metric.go
sed -n '70,120p' internal/cmd/service/metric/metric.goRepository: zeabur/cli
Length of output: 1372
🏁 Script executed:
#!/bin/bash
# Check if there's an interactive mode that might bypass ValidArgs
rg -n "RunE|PersistentPreRunE|PreRunE" internal/cmd/service/metric/metric.go -B 5 -A 10Repository: zeabur/cli
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Check the full command structure to understand how opts.metricType is set
sed -n '20,60p' internal/cmd/service/metric/metric.goRepository: zeabur/cli
Length of output: 1174
🏁 Script executed:
#!/bin/bash
# Check the interactive path to see if it also has the same pattern
sed -n '60,75p' internal/cmd/service/metric/metric.goRepository: zeabur/cli
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Verify how the flag value flows through the code - check if there's any normalization
sed -n '45,75p' internal/cmd/service/metric/metric.goRepository: zeabur/cli
Length of output: 960
Normalize MetricType construction to handle lowercase input from the --metric-type flag.
The --metric-type flag has no input validation, allowing users to pass lowercase values (e.g., --metric-type cpu). While upperCaseMetricType is correctly normalized for the API call, mt is constructed from the raw input. The WithMeasureUnit switch only matches uppercase constants ("CPU", "MEMORY", "NETWORK"), so lowercase input would skip all cases and return the unformatted value. Use the normalized value when constructing mt:
upperCaseMetricType := strings.ToUpper(opts.metricType)
mt := model.MetricType(upperCaseMetricType)📝 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.
Suggested change
| if opts.id == "" && opts.name != "" { | |
| service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) | |
| if err != nil { | |
| return err | |
| } | |
| opts.environmentID = envID | |
| opts.id = service.ID | |
| } | |
| if err := paramCheck(opts); err != nil { | |
| return err | |
| if opts.id == "" { | |
| return fmt.Errorf("--id or --name is required") | |
| } | |
| // if name is set, get service id by name | |
| if opts.id == "" && opts.name != "" { | |
| service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) | |
| // Resolve environment and project from the service | |
| service, err := f.ApiClient.GetService(context.Background(), opts.id, "", "", "") | |
| if err != nil { | |
| return fmt.Errorf("get service failed: %w", err) | |
| } | |
| projectID := "" | |
| if service.Project != nil { | |
| projectID = service.Project.ID | |
| } | |
| if opts.environmentID == "" { | |
| envID, err := util.ResolveEnvironmentID(f.ApiClient, projectID) | |
| if err != nil { | |
| return err | |
| } | |
| opts.id = service.ID | |
| opts.environmentID = envID | |
| } | |
| upperCaseMetricType := strings.ToUpper(opts.metricType) | |
| if opts.metricType == "" { | |
| return fmt.Errorf("metric type is required") | |
| } | |
| upperCaseMetricType := strings.ToUpper(opts.metricType) | |
| mt := model.MetricType(opts.metricType) | |
| startTime := time.Now().Add(-time.Duration(opts.hour) * time.Hour) | |
| endTime := time.Now() | |
| metrics, err := f.ApiClient.ServiceMetric(context.Background(), opts.id, f.Config.GetContext().GetProject().GetID(), opts.environmentID, upperCaseMetricType, startTime, endTime) | |
| metrics, err := f.ApiClient.ServiceMetric(context.Background(), opts.id, projectID, opts.environmentID, upperCaseMetricType, startTime, endTime) | |
| if opts.id == "" && opts.name != "" { | |
| service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) | |
| if err != nil { | |
| return err | |
| } | |
| opts.id = service.ID | |
| } | |
| if opts.id == "" { | |
| return fmt.Errorf("--id or --name is required") | |
| } | |
| // Resolve environment and project from the service | |
| service, err := f.ApiClient.GetService(context.Background(), opts.id, "", "", "") | |
| if err != nil { | |
| return fmt.Errorf("get service failed: %w", err) | |
| } | |
| projectID := "" | |
| if service.Project != nil { | |
| projectID = service.Project.ID | |
| } | |
| if opts.environmentID == "" { | |
| envID, err := util.ResolveEnvironmentID(f.ApiClient, projectID) | |
| if err != nil { | |
| return err | |
| } | |
| opts.environmentID = envID | |
| } | |
| if opts.metricType == "" { | |
| return fmt.Errorf("metric type is required") | |
| } | |
| upperCaseMetricType := strings.ToUpper(opts.metricType) | |
| mt := model.MetricType(upperCaseMetricType) | |
| startTime := time.Now().Add(-time.Duration(opts.hour) * time.Hour) | |
| endTime := time.Now() | |
| metrics, err := f.ApiClient.ServiceMetric(context.Background(), opts.id, projectID, opts.environmentID, upperCaseMetricType, startTime, endTime) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/service/metric/metric.go` around lines 75 - 116, The MetricType
is constructed from raw opts.metricType so lowercase flag values bypass the
WithMeasureUnit switch; change the construction to use the normalized value by
replacing mt := model.MetricType(opts.metricType) with mt :=
model.MetricType(upperCaseMetricType) (and optionally validate allowed values)
so WithMeasureUnit and any switches on MetricType (referencing mt and the
constants "CPU","MEMORY","NETWORK") will match correctly.
Comment on lines
+93
to
102
| if opts.region != "" && opts.projectID == "" { | ||
| project, err := f.ApiClient.CreateProject(context.Background(), opts.region, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("create project in region %s: %w", opts.region, err) | ||
| } | ||
| opts.projectID = project.ID | ||
| f.Log.Infof("Created project %q in region %s.", project.ID, opts.region) | ||
| } else if _, err := f.ParamFiller.ProjectCreatePreferred(&opts.projectID); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Consider warning when --region is provided alongside --project-id.
When both --region and --project-id are specified, the region flag is silently ignored since the condition on line 93 requires projectID == "". This could confuse users who expect the region to be validated against the existing project.
Consider adding explicit handling:
💡 Proposed fix to warn users
+ if opts.region != "" && opts.projectID != "" {
+ f.Log.Warnf("Both --region and --project-id provided; --region will be ignored.")
+ }
+
if opts.region != "" && opts.projectID == "" {
project, err := f.ApiClient.CreateProject(context.Background(), opts.region, 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.
Suggested change
| if opts.region != "" && opts.projectID == "" { | |
| project, err := f.ApiClient.CreateProject(context.Background(), opts.region, nil) | |
| if err != nil { | |
| return fmt.Errorf("create project in region %s: %w", opts.region, err) | |
| } | |
| opts.projectID = project.ID | |
| f.Log.Infof("Created project %q in region %s.", project.ID, opts.region) | |
| } else if _, err := f.ParamFiller.ProjectCreatePreferred(&opts.projectID); err != nil { | |
| return err | |
| } | |
| if opts.region != "" && opts.projectID != "" { | |
| f.Log.Warnf("Both --region and --project-id provided; --region will be ignored.") | |
| } | |
| if opts.region != "" && opts.projectID == "" { | |
| project, err := f.ApiClient.CreateProject(context.Background(), opts.region, nil) | |
| if err != nil { | |
| return fmt.Errorf("create project in region %s: %w", opts.region, err) | |
| } | |
| opts.projectID = project.ID | |
| f.Log.Infof("Created project %q in region %s.", project.ID, opts.region) | |
| } else if _, err := f.ParamFiller.ProjectCreatePreferred(&opts.projectID); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/template/deploy/deploy.go` around lines 93 - 102, Add an
explicit check for the case when both opts.region and opts.projectID are
provided and surface a warning (or validation) instead of silently ignoring
region: before the existing if block that creates a project, add logic that if
opts.region != "" && opts.projectID != "" then either (a) log a warning via
f.Log.Warnf that the --region flag will be ignored because --project-id was
provided, or (b) perform validation by calling f.ApiClient.GetProject(ctx,
opts.projectID) and compare the returned project.Region to opts.region and
return an error if they differ; reference opts.region, opts.projectID, f.Log and
f.ApiClient.GetProject/GetProjectByID (or CreateProject) when implementing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
zeabur service execcommand to run commands inside service containers via theexecuteCommandGraphQL mutationResolveEnvironmentIDByServiceIDhelper tointernal/util/env.goMotivation
The old design resolved environment IDs from the project context, which could point to a completely different project than the service belongs to, causing
NOT_RUNNING_SERVICEerrors even when the service is active.Now: service ID → fetch service → get project ID → resolve environment. No more wrong env IDs.
Changed commands (32 files)
service exec— execute commands in service containersTest plan
go build ./...andgo test ./...passzeabur service exec --id <svc> -- lsresolves env from service's projectzeabur service restart --id <svc>no longer requires project context🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
service execcommand to execute commands inside service containers with results and exit codes.--regionflag to template deployment for automatic project creation in specified regions.Refactor