feat(cmd): Add '--rollout' flag when creating#309
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for a --rollout mode to instance create, intended to replace existing instances in the same service group that use the same image “base name”, while also adjusting how service-group and volume identifiers are parsed during instance creation.
Changes:
- Add
--rolloutflag toinstance createand route execution through a newrunRolloutworkflow. - Modify parsing behavior for
--serviceand--volume-style inputs (notablyInstanceService.UnmarshalTextandInstanceVolume.UnmarshalText). - Remove the client-side “volume metro mismatch” validation during instance creation.
Comments suppressed due to low confidence (1)
internal/cmd/instances.go:783
- The volume creation path no longer validates
vol.Metroagainst the instance metro (unlike the service-group check below). If a volume is specified via structured input (--set volumes.0.metro=..., JSON/YAML, or if text parsing supportsmetro/...), the CLI will now proceed and fail later with a less actionable API error; consider restoring the metro-mismatch validation for volumes whenvol.Metrois set.
case "volumes":
for _, vol := range field.Create.Set.([]*InstanceVolume) {
reqVol := platform.CreateInstanceRequestVolume{
At: vol.At,
}
if vol.UUID != "" {
reqVol.Uuid = &vol.UUID
}
if vol.Name != "" {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
926d9db to
ce661f7
Compare
8ccf7d3 to
c3adcf6
Compare
|
I reworked the implementation as you asked @jedevc Right now only instance rollout is implemented, but we can do other also I think |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
internal/cmd/instances.go:904
- The volume link type (Link[Volume]) includes a Metro field, but the create path no longer validates that an explicitly metro-qualified volume matches the instance metro. Without this check, a user can specify e.g. "sfo/myvol" while creating in another metro and the metro will be silently ignored in the API request (only Name/UUID are sent). Consider restoring the metro-mismatch validation (similar to the service group check) or explicitly rejecting metro-qualified volume references here.
case "volumes":
for _, vol := range field.Create.Set.([]*InstanceVolume) {
reqVol := platform.CreateInstanceRequestVolume{
At: vol.At,
}
if vol.UUID != "" {
reqVol.Uuid = &vol.UUID
}
if vol.Name != "" {
reqVol.Name = &vol.Name
}
if vol.Size > 0 {
reqVol.SizeMb = new(uint64(vol.Size))
}
if vol.Readonly {
reqVol.Readonly = &vol.Readonly
}
req.Volumes = append(req.Volumes, reqVol)
}
143ed09 to
89cd18a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
internal/cmd/instances.go:1229
- Calling
Instance{}.Deletedirectly bypasses the sandbox wrappers used by resource commands, so--rolloutcan delete matching service instances even when they are not tracked by the active sandbox. This breaks sandbox isolation and can remove resources that normal delete commands would reject.
delErr := (Instance{}).Delete(ctx, []resource.Resource{old})
c68b276 to
144843c
Compare
|
something funky going on with tests, probably server is misbehaving. Will need to wait till I can make sure tests pass |
| if rollout && req.Autostart != nil && !*req.Autostart { | ||
| return nil, fmt.Errorf("--rollout cannot be used with --autostart=false") | ||
| } |
There was a problem hiding this comment.
Would make no sense to rollout without starting. In general you are using rollout to ensure no outage (create instance and start -> remove old instance -> repeat)
If you create new instances stopped you are probably using rollout wrongly 😅
Still, up to you, I can remove the check (but then I probably also need to remove some other checks)
There was a problem hiding this comment.
I'm saying this because right now we're loop-waiting for the new instance to be in running before deleting the old instance
(I can just put that whole check in an if-block and make it optional though)
There was a problem hiding this comment.
I will do that and revert depending on what you prefer
This adds sequential rolling over services Signed-off-by: Cezar Craciunoiu <[email protected]>
144843c to
a2e506a
Compare
|
One thing left to discuss here is if we're going for the loop-wait-retry logic If yes, then feel free to merge, if not, then we got to wait for the platform changes to go in I would say we leave as is because a rollout can take significantly more than a simple start or restart, si it's fine to have a more complex logic in place (at least conceptually) |
Open to UI/UX suggestions, right now it prints every new instance that was rolled
Closes: TOOL-794