Skip to content

Comments

feat(stage): add skip/unskip commands and show skipped section in list#610

Open
fabienfleureau wants to merge 3 commits intomainfrom
vk/da00-handle-skipped-s
Open

feat(stage): add skip/unskip commands and show skipped section in list#610
fabienfleureau wants to merge 3 commits intomainfrom
vk/da00-handle-skipped-s

Conversation

@fabienfleureau
Copy link
Collaborator

@fabienfleureau fabienfleureau commented Feb 23, 2026

Summary

  • Add environment stage skip and environment stage unskip commands to manage skipped service stages
  • Update environment stage list to display a dedicated Skipped section alongside the existing stage list
  • Refactor error handling in skip/unskip commands to use utils.CheckErr

Test plan

  • Run qovery environment stage list and verify a Skipped section appears when services are skipped
  • Run qovery environment stage skip -n <name> and confirm service is moved to skipped
  • Run qovery environment stage unskip -n <name> and confirm service is restored

Copilot AI review requested due to automatic review settings February 23, 2026 15:26
@fabienfleureau fabienfleureau force-pushed the vk/da00-handle-skipped-s branch from bc8efa8 to 159697a Compare February 23, 2026 15:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 skip and unskip commands to mark services as excluded from environment-level deployments
  • Updated list command to show skipped services in a separate section and filter them from stage listings
  • Introduced utils.CheckErr function 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
Comment on lines 17 to 23
// 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)
}
}
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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)
}
}

Copilot uses AI. Check for mistakes.
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.",
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.",

Copilot uses AI. Check for mistakes.
Comment on lines +127 to +130
j, err := json.Marshal(map[string]interface{}{
"skipped_services": skippedServices,
"stages": results,
})
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
j, err := json.Marshal(map[string]interface{}{
"skipped_services": skippedServices,
"stages": results,
})
j, err := json.Marshal(results)

Copilot uses AI. Check for mistakes.
var service *qovery.DeploymentStageServiceResponse
var currentStageId string
for _, stage := range stages.GetResults() {
service, _ = getServiceByName(client, stage.GetServices(), serviceName)
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
}

Copilot uses AI. Check for mistakes.

if len(stage.GetServices()) == 0 {
if len(data) == 0 {
utils.Println("<no service>")
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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>")
}

Copilot uses AI. Check for mistakes.
Comment on lines 42 to 43
os.Exit(1)
panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
os.Exit(1)
panic("unreachable") // staticcheck false positive: https://staticcheck.io/docs/checks#SA5011

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 67
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")
}
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
import (
"context"
"errors"
"os"
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
import (
"context"
"errors"
"os"
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
var service *qovery.DeploymentStageServiceResponse
var currentStageId string
for _, stage := range stages.GetResults() {
service, _ = getServiceByName(client, stage.GetServices(), serviceName)
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant