-
Notifications
You must be signed in to change notification settings - Fork 38
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.
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
feat(db/create): ask to wake sleeping groups before create #900
Conversation
The original issue #894 also stated that spinner.Stop()
awake, err := ensureGroupAwake(client, db.Group)
if err != nil {
return err
}
if !awake {
return fmt.Errorf("cannot connect to a database in an archived group. Please wake up the group first")
}
spinner.Start() |
I pushed a commit to |
internal/cmd/group_utils.go
Outdated
"github.com/tursodatabase/turso-cli/internal/turso" | ||
) | ||
|
||
func ensureGroupAwake(client *turso.Client, groupName string) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this approach because:
- We add an extra call on all invocations to handle an edge case that happens once in a while
- We have to keep track of all ops that are affected by archiving and remember to add this logic to them (e.g. rotating creds, and deleting dbs also doesn't work when the group is archived)
The "better" way would be to be able to detect archived errors from API responses and handle those properly.
Not a blocker though, feel free to move forward if you don't have the time to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this would be better.
} | ||
|
||
errMsg := actionErr.Error() | ||
if strings.Contains(errMsg, "group_sleeping") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear group_sleeping
will never be reached because we don't actually return or formalise the API error from the turso
client.
|
||
errMsg := actionErr.Error() | ||
if strings.Contains(errMsg, "group_sleeping") || | ||
(strings.Contains(errMsg, "cannot create database on group") && strings.Contains(errMsg, "because it is archived")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be reached, but I hate having to pass in the string and detect errors like this. I would much prefer to use group_sleeping
from the error response code
field.
Fixes #894
This does however use a mix of the
sleeping
andarchived
terminology. We should clean that up when we know which word we want to go with.