feat(vpc-gw): add interactive question to delete the IP when deleting gw#5296
feat(vpc-gw): add interactive question to delete the IP when deleting gw#5296remyleone wants to merge 3 commits intoscaleway:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5296 +/- ##
==========================================
- Coverage 55.86% 55.82% -0.05%
==========================================
Files 317 317
Lines 71645 71708 +63
==========================================
Hits 40028 40028
- Misses 30123 30186 +63
Partials 1494 1494 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds an interactive confirmation prompt when deleting a VPC gateway to optionally delete the attached IP as part of the delete operation.
Changes:
- Overrides
vpc-gw gateway deleteto support a tri-statewith-ip={prompt,true,false}flag and prompt when applicable. - Fetches gateway details prior to deletion to determine whether an IP is attached and whether prompting is needed.
- Updates CLI docs to reflect the new
with-ipargument.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/namespaces/vpcgw/v2/custom_gateway.go | Adds a custom delete command implementation with interactive IP deletion confirmation. |
| internal/namespaces/vpcgw/v2/custom.go | Wires the generated gateway delete command to the new custom builder. |
| docs/commands/vpc-gw.md | Updates documented args for vpc-gw gateway delete to use with-ip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get gateway info to check if it has an IP | ||
| gateway, err := api.GetGateway(&vpcgw.GetGatewayRequest{ | ||
| Zone: args.Zone, | ||
| GatewayID: args.GatewayID, | ||
| }) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
GetGateway is called unconditionally before deleting. If with-ip is explicitly true or false, this extra API call is unnecessary and can introduce extra latency / permission failures. Consider only fetching the gateway (and prompting) when with-ip=prompt is selected (or when using the deprecated alias, if kept).
| // Get gateway info to check if it has an IP | |
| gateway, err := api.GetGateway(&vpcgw.GetGatewayRequest{ | |
| Zone: args.Zone, | |
| GatewayID: args.GatewayID, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| var gateway *vpcgw.Gateway | |
| var err error | |
| // Only fetch gateway info when we may need to prompt about deleting the IP | |
| if args.WithIP == withIPPrompt { | |
| gateway, err = api.GetGateway(&vpcgw.GetGatewayRequest{ | |
| Zone: args.Zone, | |
| GatewayID: args.GatewayID, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } |
| default: | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
In shouldDeleteIP, the default case silently returns (false, nil). This can hide unexpected values and change behavior without surfacing an error. Other tri-state flags in the codebase return an error for unsupported values (e.g. internal/namespaces/instance/v1/custom_server_action.go with-block handling). Return a descriptive error here instead of silently defaulting.
| | gateway-id | Required | ID of the gateway to delete | | ||
| | delete-ip | | Defines whether the PGW's IP should be deleted | | ||
| | with-ip | Default: `prompt`<br />One of: `prompt`, `true`, `false` | Delete the IP attached to the gateway | | ||
| | zone | Default: `fr-par-1`<br />One of: `fr-par-1`, `fr-par-2`, `nl-ams-1`, `nl-ams-2`, `nl-ams-3`, `pl-waw-1`, `pl-waw-2`, `pl-waw-3` | Zone to target. If none is passed will use default zone from the config | |
There was a problem hiding this comment.
Docs replace delete-ip with with-ip. If delete-ip is already released, this is a breaking CLI change for users following existing docs/scripts; consider documenting delete-ip as a deprecated alias (or keeping it) to preserve backward compatibility.
| type customDeleteGatewayRequest struct { | ||
| Zone scw.Zone | ||
| GatewayID string | ||
| DeleteIP bool |
There was a problem hiding this comment.
customDeleteGatewayRequest defines DeleteIP but the field is never read and there is no ArgSpec for delete-ip, so it is currently dead code. Either remove this field, or reintroduce delete-ip as a deprecated alias (ideally as *bool so you can distinguish “not set” from “set to false”) and map it to the new behavior.
| DeleteIP bool |
| Name: "with-ip", | ||
| Short: "Delete the IP attached to the gateway", | ||
| Default: core.DefaultValueSetter(withIPPrompt), | ||
| EnumValues: []string{ | ||
| withIPPrompt, | ||
| withIPTrue, | ||
| withIPFalse, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This override renames the CLI arg from delete-ip (generated command) to with-ip, which will break existing scripts. Consider keeping delete-ip as a deprecated alias (e.g., same OneOfGroup as with-ip to prevent both being set) and translating it to with-ip=true/false behavior internally.
| Name: "with-ip", | |
| Short: "Delete the IP attached to the gateway", | |
| Default: core.DefaultValueSetter(withIPPrompt), | |
| EnumValues: []string{ | |
| withIPPrompt, | |
| withIPTrue, | |
| withIPFalse, | |
| }, | |
| }, | |
| Name: "with-ip", | |
| Short: "Delete the IP attached to the gateway", | |
| Default: core.DefaultValueSetter(withIPPrompt), | |
| EnumValues: []string{ | |
| withIPPrompt, | |
| withIPTrue, | |
| withIPFalse, | |
| }, | |
| OneOfGroup: "gateway-delete-ip-mode", | |
| }, | |
| { | |
| Name: "delete-ip", | |
| Short: "Delete the IP attached to the gateway (deprecated, use with-ip instead)", | |
| Deprecated: "use with-ip instead", | |
| OneOfGroup: "gateway-delete-ip-mode", | |
| }, |
| |------|---|-------------| | ||
| | gateway-id | Required | ID of the gateway to delete | | ||
| | delete-ip | | Defines whether the PGW's IP should be deleted | | ||
| | with-ip | Default: `prompt`<br />One of: `prompt`, `true`, `false` | Delete the IP attached to the gateway | |
There was a problem hiding this comment.
Maybe there is a way to avoid a breaking change here
Community Note
Relates OR Closes #0000
Release note for CHANGELOG: