Add show_deleted support to SP resource list and get commands#20
Conversation
## What Add `--show-deleted` flag to SP resource list and get commands to include soft-deleted resources in results. ## Why The service-provider-manager resource API now supports a `show_deleted` query parameter that allows retrieving soft-deleted instances alongside active ones. The CLI needs to expose this capability to users. ## Changes - Updated service-provider-manager dependency to pick up the new `ShowDeleted` param on list/get and the `GetInstanceParams` type - Added `--show-deleted` boolean flag to both `sp resource list` and `sp resource get` - When `--show-deleted` is set, passes `show_deleted=true` query parameter to the API - When `--show-deleted` is set, displays an additional `DELETION STATUS` column in table output - Updated import paths for provider packages to match upstream restructuring ## Testing Added unit tests verifying the `show_deleted` query parameter is sent for both list and get, and that the `DELETION STATUS` column appears only when the flag is used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
requested review from
ebichman-1,
gabriel-farache,
gciavarrini,
jenniferubah and
machacekondra
as code owners
## What Updates project documentation to cover the new --show-deleted flag on SP resource commands. ## Why The implementation added in the previous commit needs corresponding spec requirements, test plan entries, and a checkpoint for traceability. ## Changes - Added REQ-SPR-035/060/070 and AC-SPR-035/036/045/046 to the spec for show-deleted behavior - Updated table output section in the spec to show both default and --show-deleted column layouts - Added TC-U154–TC-U157 to the unit test plan covering list and get with/without --show-deleted - Updated coverage matrix and test case totals - Created checkpoint documenting the feature scope, design decisions, and files changed ## Testing No new code tests — documentation only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
| | 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 | | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Yes, but in the current implementation a deletion status may also be Failed since the SPRM has a retry limit
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
gciavarrini
left a comment
gciavarrini
left a comment
There was a problem hiding this comment.
small nits, but overall LGTM
Add ID verification to Get and List calls Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
force-pushed
the
add-show-deleted-sp-resource
branch
from
1814091 to
250467b
Compare
5 participants
What
Add
--show-deletedflag to SP resource list and get commands to include soft-deleted resources in results.Why
The service-provider-manager resource API now supports a
show_deletedquery parameter that allows retrieving soft-deleted instances alongside active ones. The CLI needs to expose this capability to users.Changes
ShowDeletedparam on list/get and theGetInstanceParamstype--show-deletedboolean flag to bothsp resource listandsp resource get--show-deletedis set, passesshow_deleted=truequery parameter to the API--show-deletedis set, displays an additionalDELETION STATUScolumn in table outputTesting
Added unit tests verifying the
show_deletedquery parameter is sent for both list and get, and that theDELETION STATUScolumn appears only when the flag is used.