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
dolt_clone stored procedure #3745
Conversation
This needs tests but I'm not going to get to them before taking off on vacation. I'm putting it up for review because Jennifer has already merged some of these changes into her own branch, so it seems wise to get it checked in before it diverges too much. I can write bats tests when I'm back in town. |
Maybe Jennifer can write the tests for you?
…On Thu, Jul 14, 2022 at 3:03 PM Zach Musgrave ***@***.***> wrote:
This needs tests but I'm not going to get to them before taking off on
vacation. I'm putting it up for review because Jennifer has already merged
some of these changes into her own branch, so it seems wise to get it
checked in before it diverges too much. I can write bats tests when I'm
back in town.
—
Reply to this email directly, view it on GitHub
<#3745 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJAR3CHTLCYKEHXLXB7THTVUCFCLANCNFSM52N5TXCQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Actually did add a bats test, but it needs more |
I'll start digging into this one, review, and add some more tests so we can unblock Jennifer's dolt_remote change. |
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.
Looks pretty good. I noticed a couple spots where some error handling is needed. I'll flesh out the BATS tests a little more and fill in the error handling, then get this merged in and add some docs.
…sn't a direct CLI command (and would cause SQL command usage to print); cleaning up whitespace.
… a copy instead of modifying the original struct, like their docs describe.
… working directory and causing incorrect behavior after running dolt_clone().
…g remote name, and cloning from DoltHub)
…ng a single branch.
@@ -212,6 +215,87 @@ func (p DoltDatabaseProvider) CreateDatabase(ctx *sql.Context, name string) erro | |||
return dsess.AddDB(ctx, dbstate) | |||
} | |||
|
|||
// CloneDatabaseFromRemote implements DoltDatabaseProvider interface | |||
func (p DoltDatabaseProvider) CloneDatabaseFromRemote(ctx *sql.Context, dbName, branch, remoteName, remoteUrl string, remoteParams map[string]string) error { | |||
p.mu.Lock() |
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 is potentially a loooong time to be holding a lock! I think this is okay for v1, since dolt_clone()
will not be frequently used, but it would be good to follow up and shrink down the code that executes with this lock.
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.
LGTM
Adds support for a new
dolt_clone()
stored procedure.Removes most of the use of
os.Chdir
(except for in test code and one or two dolt-cli-only spots).Doc Updates: dolthub/docs#804