feat(stage): add skip/unskip commands and show skipped section in list#610
feat(stage): add skip/unskip commands and show skipped section in list#610fabienfleureau wants to merge 3 commits intomainfrom
Conversation
bc8efa8 to
159697a
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds skip/unskip functionality for service stages in the Qovery CLI, allowing users to exclude services from environment-level bulk deployments while keeping them in their current stage. The PR introduces two new commands (environment stage skip and environment stage unskip), updates the list command to display skipped services in a dedicated section, and refactors error handling using a new utils.CheckErr helper function.
Changes:
- Added
skipandunskipcommands to mark services as excluded from environment-level deployments - Updated
listcommand to show skipped services in a separate section and filter them from stage listings - Introduced
utils.CheckErrfunction for standardized error handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/error.go | Added new CheckErr helper function for error handling |
| cmd/environment_stage_skip.go | New command to mark a service as skipped |
| cmd/environment_stage_unskip.go | New command to remove skipped status from a service |
| cmd/environment_stage_list.go | Updated to display skipped services separately and use CheckErr for error handling |
| docs/plans/2026-02-23-skipped-service-stage.md | Implementation plan document |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
utils/error.go
Outdated
| // CheckErr prints the error and exits with code 1 if err is non-nil. | ||
| func CheckErr(err error) { | ||
| if err != nil { | ||
| PrintlnError(err) | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new CheckErr function duplicates functionality already provided by the existing utils.CheckError function in utils/qovery.go. The existing CheckError function includes a panic statement after os.Exit to satisfy staticcheck, while this new function does not. Consider using the existing utils.CheckError instead, or if you prefer the shorter name, consider renaming or removing the existing CheckError to avoid confusion.
| // CheckErr prints the error and exits with code 1 if err is non-nil. | |
| func CheckErr(err error) { | |
| if err != nil { | |
| PrintlnError(err) | |
| os.Exit(1) | |
| } | |
| } | |
| // CheckErr is a helper that delegates to CheckError for error handling. | |
| func CheckErr(err error) { | |
| CheckError(err) | |
| } | |
| } |
cmd/environment_stage_skip.go
Outdated
| var environmentStageSkipCmd = &cobra.Command{ | ||
| Use: "skip", | ||
| Short: "Skip service from environment-level deployments", | ||
| Long: "Mark a service as skipped so it is excluded from environment-level bulk deployments while staying in its current stage. Use 'environment stage unskip' to reverse.", |
There was a problem hiding this comment.
The Long description references "environment stage unskip" command which is consistent with the actual implementation. However, the plan document mentioned that moving a service would automatically unskip it. Consider clarifying in the help text that both methods are available: using "unskip" explicitly or moving the service to a different stage.
| Long: "Mark a service as skipped so it is excluded from environment-level bulk deployments while staying in its current stage. Use 'environment stage unskip' to reverse.", | |
| Long: "Mark a service as skipped so it is excluded from environment-level bulk deployments while staying in its current stage. To reverse this, you can either use 'environment stage unskip' or move the service to a different deployment stage, which will automatically clear the skipped status.", |
| j, err := json.Marshal(map[string]interface{}{ | ||
| "skipped_services": skippedServices, | ||
| "stages": results, | ||
| }) |
There was a problem hiding this comment.
The JSON output structure has changed from an array of stages to an object with "skipped_services" and "stages" properties. This is a breaking change for any scripts or tools that parse the JSON output of "qovery environment stage list --json". Consider maintaining backward compatibility by keeping the original structure, or document this breaking change clearly in the release notes.
| j, err := json.Marshal(map[string]interface{}{ | |
| "skipped_services": skippedServices, | |
| "stages": results, | |
| }) | |
| j, err := json.Marshal(results) |
| var service *qovery.DeploymentStageServiceResponse | ||
| var currentStageId string | ||
| for _, stage := range stages.GetResults() { | ||
| service, _ = getServiceByName(client, stage.GetServices(), serviceName) |
There was a problem hiding this comment.
Potential error silently ignored. The error returned by getServiceByName is being ignored (line 33). If an error occurs while fetching service details (e.g., API failure), the service will be nil, and the code correctly handles that at line 40. However, it would be more transparent to check the error and provide a more specific error message when the API call fails versus when the service simply isn't found in the list. This matches the existing pattern in environment_stage_move.go:44, but consider improving both.
| service, _ = getServiceByName(client, stage.GetServices(), serviceName) | |
| service, err = getServiceByName(client, stage.GetServices(), serviceName) | |
| if err != nil { | |
| utils.CheckErr(err) | |
| os.Exit(1) | |
| panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011 | |
| } |
cmd/environment_stage_list.go
Outdated
|
|
||
| if len(stage.GetServices()) == 0 { | ||
| if len(data) == 0 { | ||
| utils.Println("<no service>") |
There was a problem hiding this comment.
Consider clarifying the no service message. When a stage has only skipped services, it now displays "<no service>" which could be confusing since there are services in that stage, they're just all skipped. Consider a message like "<all services skipped>" or "<no active services>" for better clarity.
| utils.Println("<no service>") | |
| // Distinguish between stages with no services at all and stages where all services are skipped | |
| if len(stage.GetServices()) == 0 { | |
| utils.Println("<no service>") | |
| } else { | |
| utils.Println("<all services skipped>") | |
| } |
cmd/environment_stage_unskip.go
Outdated
| os.Exit(1) | ||
| panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011 |
There was a problem hiding this comment.
Unreachable code after CheckErr. The CheckErr function at line 41 already calls os.Exit(1) when the error is non-nil, so the os.Exit(1) and panic on lines 42-43 will never be reached. Remove these lines since CheckErr handles program termination.
| os.Exit(1) | |
| panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011 |
| package cmd | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" | ||
|
|
||
| "github.com/qovery/qovery-cli/utils" | ||
| "github.com/qovery/qovery-client-go" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| var environmentStageUnskipCmd = &cobra.Command{ | ||
| Use: "unskip", | ||
| Short: "Unskip service from environment-level deployments", | ||
| Long: "Remove the skipped flag from a service so it is included again in environment-level bulk deployments.", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| utils.Capture(cmd) | ||
|
|
||
| tokenType, token, err := utils.GetAccessToken() | ||
| utils.CheckErr(err) | ||
|
|
||
| client := utils.GetQoveryClient(tokenType, token) | ||
| _, _, environmentId, err := getOrganizationProjectEnvironmentContextResourcesIds(client) | ||
| utils.CheckErr(err) | ||
|
|
||
| stages, _, err := client.DeploymentStageMainCallsAPI.ListEnvironmentDeploymentStage(context.Background(), environmentId).Execute() | ||
| utils.CheckErr(err) | ||
|
|
||
| var service *qovery.DeploymentStageServiceResponse | ||
| var currentStageId string | ||
| for _, stage := range stages.GetResults() { | ||
| service, _ = getServiceByName(client, stage.GetServices(), serviceName) | ||
| if service != nil { | ||
| currentStageId = stage.GetId() | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if service == nil { | ||
| utils.CheckErr(errors.New("service not found")) | ||
| os.Exit(1) | ||
| panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011 | ||
| } | ||
|
|
||
| req := qovery.AttachServiceToDeploymentStageRequest{} | ||
| req.SetIsSkipped(false) | ||
|
|
||
| _, _, err = client.DeploymentStageMainCallsAPI. | ||
| AttachServiceToDeploymentStage(context.Background(), currentStageId, service.GetServiceId()). | ||
| AttachServiceToDeploymentStageRequest(req). | ||
| Execute() | ||
| utils.CheckErr(err) | ||
|
|
||
| utils.Println("Service \"" + serviceName + "\" is no longer skipped from environment-level deployments") | ||
| }, | ||
| } | ||
|
|
||
| func init() { | ||
| environmentStageCmd.AddCommand(environmentStageUnskipCmd) | ||
| environmentStageUnskipCmd.Flags().StringVarP(&organizationName, "organization", "", "", "Organization Name") | ||
| environmentStageUnskipCmd.Flags().StringVarP(&projectName, "project", "", "", "Project Name") | ||
| environmentStageUnskipCmd.Flags().StringVarP(&environmentName, "environment", "", "", "Environment Name") | ||
| environmentStageUnskipCmd.Flags().StringVarP(&serviceName, "name", "n", "", "Service Name") | ||
|
|
||
| _ = environmentStageUnskipCmd.MarkFlagRequired("name") | ||
| } |
There was a problem hiding this comment.
The implementation deviates from the documentation in docs/plans/2026-02-23-skipped-service-stage.md. The plan document (line 19) states "no separate unskip command needed" and describes that moving a service automatically un-skips it. However, this PR includes a new unskip command. Consider updating the documentation to reflect the actual implementation, or clarify why both mechanisms (move to unskip, and explicit unskip command) are needed.
cmd/environment_stage_skip.go
Outdated
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" |
There was a problem hiding this comment.
Unused import. The "os" package is only imported for the unreachable os.Exit(1) call on line 42. After removing the unreachable code (lines 42-43), this import should also be removed.
cmd/environment_stage_unskip.go
Outdated
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" |
There was a problem hiding this comment.
Unused import. The "os" package is only imported for the unreachable os.Exit(1) call on line 42. After removing the unreachable code (lines 42-43), this import should also be removed.
| var service *qovery.DeploymentStageServiceResponse | ||
| var currentStageId string | ||
| for _, stage := range stages.GetResults() { | ||
| service, _ = getServiceByName(client, stage.GetServices(), serviceName) |
There was a problem hiding this comment.
Potential error silently ignored. The error returned by getServiceByName is being ignored (line 33). If an error occurs while fetching service details (e.g., API failure), the service will be nil, and the code correctly handles that at line 40. However, it would be more transparent to check the error and provide a more specific error message when the API call fails versus when the service simply isn't found in the list. This matches the existing pattern in environment_stage_move.go:44, but consider improving both.
…ed-only stage message
Summary
environment stage skipandenvironment stage unskipcommands to manage skipped service stagesenvironment stage listto display a dedicated Skipped section alongside the existing stage listutils.CheckErrTest plan
qovery environment stage listand verify a Skipped section appears when services are skippedqovery environment stage skip -n <name>and confirm service is moved to skippedqovery environment stage unskip -n <name>and confirm service is restored