Skip to content

Add show_deleted support to SP resource list and get commands #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ygalblum merged 3 commits into from
Apr 10, 2026
Merged

Add show_deleted support to SP resource list and get commands #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d0e332
Add show_deleted support to SP resource list and get commands
ygalblum Apr 9, 2026
a3df3ee
Add spec, test plan, and checkpoint for show_deleted SP resource support
ygalblum Apr 9, 2026
250467b
Unit tests - Verify IDs
ygalblum Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions .ai/checkpoints/sp-resource-show-deleted.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 46 additions & 3 deletions .ai/specs/dcm-cli.spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,19 +1018,30 @@ SP health check.

| ID | Requirement | Priority | Notes |
|----|-------------|----------|-------|
| REQ-SPR-010 | `dcm sp resource list` MUST list SP resources (service type instances) with optional `--provider`, `--page-size`, `--page-token` flags | MUST | |
| REQ-SPR-010 | `dcm sp resource list` MUST list SP resources (service type instances) with optional `--provider`, `--show-deleted`, `--page-size`, `--page-token` flags | MUST | |
| REQ-SPR-020 | `dcm sp resource list` MUST display SP resources in the configured output format | MUST | |
| REQ-SPR-030 | `dcm sp resource get` MUST accept an `INSTANCE_ID` positional argument and display the SP resource | MUST | |
| REQ-SPR-035 | `dcm sp resource get` MUST support an optional `--show-deleted` flag | MUST | |
| REQ-SPR-040 | Missing `INSTANCE_ID` argument for `get` MUST result in a usage error (exit code 2) | MUST | |
| REQ-SPR-050 | All SP resource commands MUST use the generated SP Resource Manager client | MUST | |
| REQ-SPR-060 | When `--show-deleted` is set, `list` and `get` MUST pass `show_deleted=true` as a query parameter to the API | MUST | |
| REQ-SPR-070 | When `--show-deleted` is set, table output MUST include an additional `DELETION STATUS` column displaying the `deletion_status` field | MUST | |

@gabriel-farache gabriel-farache Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really like to add a new column DELETION STATUS, IMO the already present STATUS should be self explanatory

Now, based on my comment on another PR for the SPRM, I know that PENDING is re-used as a status for deletion which may cause collision and lost of meaning for this status (has the resource just been created or is it pending its deletion). This should be fixed in the SPRM and we should use a dedicated status when deletion is pending and thus, here we should not introduce this new column that will show empty entry for a lot of rows and that will change the layout of the response

@ygalblum ygalblum Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure I agree. I understand the desire to have a single unified status field. But, a resource can have a status (running, failed, unknown) regardless to it being pending deletion. BTW this is why K8S added the Conditions array to the status.

@gabriel-farache gabriel-farache Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see your point, then "pending deletion" would make more sense as a boolean rather than a status dedicated to delete status: when the resource is deleted it will not be returned anymore (hard delete)

@ygalblum ygalblum Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in the current implementation a deletion status may also be Failed since the SPRM has a retry limit

@gabriel-farache gabriel-farache Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, but in the current implementation a deletion status may also be Failed since the SPRM has a retry limit

right

As you mentioned K8s, why not have it displayed like an array in the status then? Also, maybe we can introduced a more generic tag to show the extended details of the resource instead of --show-deleted (which should be --show-deferred-delete)
If you feel that's not needed at this stage, it's fine

@ygalblum ygalblum Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think a lot will change in the next version. Especially if we move to async internal communication (that is event based).
Furthermore, the CLI is only meant to reflect the API (instead of writing cURL). So, any such discussion will need to take place at the service level not the CLI's


#### Table Output Columns

Default:
```
ID PROVIDER STATUS CREATED
my-instance kubevirt-123 READY 2026-03-09T10:00:00Z
```

With `--show-deleted`:
```
ID PROVIDER STATUS DELETION STATUS CREATED
my-instance kubevirt-123 READY 2026-03-09T10:00:00Z
deleted-instance kubevirt-123 DELETED PENDING 2026-03-09T10:00:00Z
```

#### Acceptance Criteria

##### AC-SPR-010: List SP resources
Expand All @@ -1055,6 +1066,22 @@ my-instance kubevirt-123 READY 2026-03-09T10:00:00Z
- **When** `dcm sp resource list --provider kubevirt-123` is invoked
- **Then** the GET request MUST include `provider=kubevirt-123` as a query parameter

##### AC-SPR-035: List SP resources with show-deleted

- **Validates:** REQ-SPR-010, REQ-SPR-060, REQ-SPR-070
- **Given** SP resources exist in the system, including soft-deleted ones
- **When** `dcm sp resource list --show-deleted` is invoked
- **Then** the GET request MUST include `show_deleted=true` as a query parameter
- **And** the table output MUST include a `DELETION STATUS` column

##### AC-SPR-036: List SP resources without show-deleted omits deletion status column

- **Validates:** REQ-SPR-070
- **Given** SP resources exist in the system
- **When** `dcm sp resource list` is invoked without `--show-deleted`
- **Then** the `show_deleted` query parameter MUST NOT be sent
- **And** the table output MUST NOT include a `DELETION STATUS` column

##### AC-SPR-040: Get SP resource

- **Validates:** REQ-SPR-030
Expand All @@ -1063,6 +1090,22 @@ my-instance kubevirt-123 READY 2026-03-09T10:00:00Z
- **Then** a GET request MUST be sent to `/api/v1alpha1/service-type-instances/my-instance`
- **And** the SP resource MUST be displayed in the configured output format

##### AC-SPR-045: Get SP resource with show-deleted

- **Validates:** REQ-SPR-035, REQ-SPR-060, REQ-SPR-070
- **Given** a soft-deleted SP resource with ID `deleted-instance` exists
- **When** `dcm sp resource get deleted-instance --show-deleted` is invoked
- **Then** the GET request MUST include `show_deleted=true` as a query parameter
- **And** the table output MUST include a `DELETION STATUS` column

##### AC-SPR-046: Get SP resource without show-deleted omits deletion status column

- **Validates:** REQ-SPR-070
- **Given** an SP resource with ID `my-instance` exists
- **When** `dcm sp resource get my-instance` is invoked without `--show-deleted`
- **Then** the `show_deleted` query parameter MUST NOT be sent
- **And** the table output MUST NOT include a `DELETION STATUS` column

##### AC-SPR-050: Get without INSTANCE_ID

- **Validates:** REQ-SPR-040
Expand Down Expand Up @@ -1651,11 +1694,11 @@ REQ-OUT-090, REQ-OUT-110, REQ-OUT-120
| REQ-CIT-NNN | 4.6: Catalog Item Commands | 11 |
| REQ-CIN-NNN | 4.7: Catalog Instance Commands | 11 |
| REQ-VER-NNN | 4.8: Version Command | 3 |
| REQ-SPR-NNN | 4.9: SP Resource Commands | 5 |
| REQ-SPR-NNN | 4.9: SP Resource Commands | 8 |
| REQ-CMP-NNN | 4.10: Shell Completion Command | 6 |
| REQ-SPP-NNN | 4.11: SP Provider Commands | 5 |
| REQ-XC-ERR-NNN | 5.1: Error Handling | 7 |
| REQ-XC-INP-NNN | 5.2: Input File Parsing | 3 |
| REQ-XC-CLI-NNN | 5.3: Generated Client Usage | 6 |
| REQ-XC-PAG-NNN | 5.4: Pagination | 3 |
| **Total** | | **106** |
| **Total** | | **109** |
45 changes: 42 additions & 3 deletions .ai/test-plans/dcm-cli-unit.test-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,24 @@ test classes. Instead:
- **When:** `dcm sp resource get my-instance` is executed
- **Then:** A GET request is sent to `/api/v1alpha1/service-type-instances/my-instance` AND the SP resource is displayed

### TC-U156: Get SP resource with show-deleted

- **Requirement:** REQ-SPR-035, REQ-SPR-060, REQ-SPR-070
- **Acceptance Criteria:** AC-SPR-045
- **Type:** Unit
- **Given:** A mock server returning 200 with a soft-deleted SP resource (with `deletion_status: "PENDING"`)
- **When:** `dcm sp resource get deleted-instance --show-deleted` is executed
- **Then:** The GET request includes `show_deleted=true` as a query parameter AND the table output includes a `DELETION STATUS` column AND the `PENDING` value is displayed

### TC-U157: Get SP resource without show-deleted omits deletion status column

- **Requirement:** REQ-SPR-070
- **Acceptance Criteria:** AC-SPR-046
- **Type:** Unit
- **Given:** A mock server returning 200 with an SP resource
- **When:** `dcm sp resource get my-instance` is executed without `--show-deleted`
- **Then:** The `show_deleted` query parameter is NOT sent AND the table output does NOT include a `DELETION STATUS` column

### TC-U125: Get SP resource without INSTANCE_ID fails

- **Requirement:** REQ-SPR-040
Expand All @@ -961,6 +979,24 @@ test classes. Instead:
- **When:** `dcm sp resource get` is executed
- **Then:** The CLI exits with code 2 and displays a usage error

### TC-U154: List SP resources with show-deleted

- **Requirement:** REQ-SPR-010, REQ-SPR-060, REQ-SPR-070
- **Acceptance Criteria:** AC-SPR-035
- **Type:** Unit
- **Given:** A mock server returning 200 with a list including a soft-deleted SP resource (with `deletion_status: "PENDING"`)
- **When:** `dcm sp resource list --show-deleted` is executed
- **Then:** The GET request includes `show_deleted=true` as a query parameter AND the table output includes a `DELETION STATUS` column AND the `PENDING` value is displayed

### TC-U155: List SP resources without show-deleted omits deletion status column

- **Requirement:** REQ-SPR-070
- **Acceptance Criteria:** AC-SPR-036
- **Type:** Unit
- **Given:** A mock server returning 200 with a list of SP resources
- **When:** `dcm sp resource list` is executed without `--show-deleted`
- **Then:** The `show_deleted` query parameter is NOT sent AND the table output does NOT include a `DELETION STATUS` column

### TC-U126: List SP resources returns empty list

- **Requirement:** REQ-SPR-010, REQ-SPR-020
Expand Down Expand Up @@ -1564,7 +1600,7 @@ dedicated test class or `Describe` block.
| REQ-OUT-020 | TC-U009 (table is default) | Covered |
| REQ-OUT-030 | TC-U017 | Covered |
| REQ-OUT-040 | TC-U009, TC-U010, TC-U018 | Covered |
| REQ-OUT-050 | TC-U009, TC-U010, TC-U041, TC-U057, TC-U079, TC-U128, TC-U146 | Covered |
| REQ-OUT-050 | TC-U009, TC-U010, TC-U041, TC-U057, TC-U079, TC-U128, TC-U146, TC-U154, TC-U156 | Covered |
| REQ-OUT-060 | TC-U011 | Covered |
| REQ-OUT-070 | TC-U012 | Covered |
| REQ-OUT-080 | TC-U014, TC-U015 | Covered |
Expand Down Expand Up @@ -1614,11 +1650,14 @@ dedicated test class or `Describe` block.
| REQ-CIN-110 | TC-U076, TC-U078, TC-U151 | Covered |
| REQ-CIN-120 | TC-U150, TC-U152, TC-U153 | Covered |
| REQ-CIN-130 | TC-U150 | Covered |
| REQ-SPR-010 | TC-U121, TC-U122, TC-U123 | Covered |
| REQ-SPR-010 | TC-U121, TC-U122, TC-U123, TC-U154 | Covered |
| REQ-SPR-020 | TC-U121 | Covered |
| REQ-SPR-030 | TC-U124 | Covered |
| REQ-SPR-035 | TC-U156 | Covered |
| REQ-SPR-040 | TC-U125 | Covered |
| REQ-SPR-050 | TC-U131 (via TC-U121, TC-U124) | Covered |
| REQ-SPR-060 | TC-U154, TC-U156 | Covered |
| REQ-SPR-070 | TC-U154, TC-U155, TC-U156, TC-U157 | Covered |
| REQ-SPP-010 | TC-U139, TC-U140, TC-U141 | Covered |
| REQ-SPP-020 | TC-U139 | Covered |
| REQ-SPP-030 | TC-U142 | Covered |
Expand Down Expand Up @@ -1662,7 +1701,7 @@ dedicated test class or `Describe` block.
| REQ-XC-TLS-070 | TC-U095, TC-U096 | Covered |
| REQ-XC-TLS-080 | TC-U090, TC-U097 | Covered |

**Total:** 117 test case IDs — 91 in behavioural test classes, 26 in the utility
**Total:** 121 test case IDs — 95 in behavioural test classes, 26 in the utility
index (tested transitively through higher-level behavioural tests).

---
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ go 1.25.5
require (
github.com/dcm-project/catalog-manager v0.0.0-20260408124701-1315c44e39b9
github.com/dcm-project/policy-manager v0.0.0-20260310132113-15bd45617e87
github.com/dcm-project/service-provider-manager v0.0.0-20260324094657-8aad860d86d2
github.com/dcm-project/service-provider-manager v0.0.0-20260402145323-bf3185a968d6
github.com/onsi/ginkgo/v2 v2.28.1
github.com/onsi/gomega v1.39.1
github.com/spf13/cobra v1.10.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/commands/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"time"

catalogclient "github.com/dcm-project/catalog-manager/pkg/client"
spmclient "github.com/dcm-project/service-provider-manager/pkg/client"
spmclient "github.com/dcm-project/service-provider-manager/pkg/client/provider"
sprmclient "github.com/dcm-project/service-provider-manager/pkg/client/resource_manager"

"github.com/dcm-project/cli/internal/config"
Expand Down
2 changes: 1 addition & 1 deletion internal/commands/sp_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"fmt"
"net/http"

spmapi "github.com/dcm-project/service-provider-manager/api/v1alpha1"
spmapi "github.com/dcm-project/service-provider-manager/api/v1alpha1/provider"

"github.com/dcm-project/cli/internal/config"
"github.com/dcm-project/cli/internal/output"
Expand Down
53 changes: 49 additions & 4 deletions internal/commands/sp_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ var spResourceTableDef = &output.TableDef{
},
}

var spResourceWithDeletedTableDef = &output.TableDef{
Headers: []string{"ID", "PROVIDER", "STATUS", "DELETION STATUS", "CREATED"},
RowFunc: func(resource any) []string {
m, ok := resource.(map[string]any)
if !ok {
return []string{"", "", "", "", ""}
}
return []string{
stringifyValue(m, "id"),
stringifyValue(m, "provider_name"),
stringifyValue(m, "status"),
stringifyValue(m, "deletion_status"),
stringifyValue(m, "create_time"),
}
},
}

func newSPResourceCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "resource",
Expand All @@ -52,7 +69,14 @@ func newSPResourceListCommand() *cobra.Command {
listCmd += fmt.Sprintf(" --page-size %d", pageSize)
}

formatter, err := newFormatter(cmd, spResourceTableDef, listCmd)
showDeleted, _ := cmd.Flags().GetBool("show-deleted")
Comment thread
gciavarrini marked this conversation as resolved.

tableDef := spResourceTableDef
if showDeleted {
tableDef = spResourceWithDeletedTableDef
}

formatter, err := newFormatter(cmd, tableDef, listCmd)
if err != nil {
return err
}
Expand All @@ -68,6 +92,9 @@ func newSPResourceListCommand() *cobra.Command {
if provider, _ := cmd.Flags().GetString("provider"); provider != "" {
params.Provider = &provider
}
if showDeleted {
params.ShowDeleted = &showDeleted
}

client, err := newSPResourceClient(cfg)
if err != nil {
Expand Down Expand Up @@ -107,22 +134,36 @@ func newSPResourceListCommand() *cobra.Command {
cmd.Flags().Int32("page-size", 0, "Maximum results per page")
cmd.Flags().String("page-token", "", "Token for next page")
cmd.Flags().String("provider", "", "Filter by provider")
cmd.Flags().Bool("show-deleted", false, "Include soft-deleted resources")

return cmd
}

func newSPResourceGetCommand() *cobra.Command {
return &cobra.Command{
cmd := &cobra.Command{
Use: "get INSTANCE_ID",
Short: "Get an SP resource by ID",
Args: ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
cfg := config.FromCommand(cmd)
formatter, err := newFormatter(cmd, spResourceTableDef, "sp resource get")

showDeleted, _ := cmd.Flags().GetBool("show-deleted")

tableDef := spResourceTableDef
if showDeleted {
tableDef = spResourceWithDeletedTableDef
}

formatter, err := newFormatter(cmd, tableDef, "sp resource get")
if err != nil {
return err
}

params := &sprmapi.GetInstanceParams{}
if showDeleted {
params.ShowDeleted = &showDeleted
}

client, err := newSPResourceClient(cfg)
if err != nil {
return fmt.Errorf("creating SP resource client: %w", err)
Expand All @@ -131,7 +172,7 @@ func newSPResourceGetCommand() *cobra.Command {
ctx, cancel := requestContext(cmd)
defer cancel()

resp, err := client.GetInstance(ctx, args[0])
resp, err := client.GetInstance(ctx, args[0], params)
if err != nil {
return connectionError(err, cfg)
}
Expand All @@ -149,4 +190,8 @@ func newSPResourceGetCommand() *cobra.Command {
return formatter.FormatOne(result)
},
}

cmd.Flags().Bool("show-deleted", false, "Show soft-deleted resource")
Comment thread
ygalblum marked this conversation as resolved.

return cmd
}
Loading