Skip to content

[WIP] Update to Gitbase v0.20.0 - #439

Open
se7entyse7en wants to merge 21 commits into
src-d:masterfrom
se7entyse7en:gitbase-v0.20.0
Open

[WIP] Update to Gitbase v0.20.0#439
se7entyse7en wants to merge 21 commits into
src-d:masterfrom
se7entyse7en:gitbase-v0.20.0

Conversation

@se7entyse7en

@se7entyse7en se7entyse7en commented Apr 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #424.

Marked as WIP as waiting for final Gitbase v0.20.0 release.

This PR removes the MysqlCli component and uses the MySql client provided by Gitbase itself.

@se7entyse7en

se7entyse7en commented Apr 16, 2019

Copy link
Copy Markdown
Contributor Author

The tests are failing due to a bug that will be fixed by #432.

@carlosms

carlosms commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

Looks good so far.
But it looks like there are more test that are failing, not only because of #432.

@se7entyse7en

se7entyse7en commented Apr 18, 2019

Copy link
Copy Markdown
Contributor Author

@carlosms thanks I didn't notice. To simplify review I've just rebased to master to solve conflicts, gonna address the tests now and also update to beta3.

smacker
smacker reviewed Apr 22, 2019
View reviewed changes
Comment thread cmd/srcd/cmd/sql.go
return humanizef(err, "could not start gitbase")
}

if err := docker.EnsureInstalled(components.MysqlCli.Image, components.MysqlCli.Version); err != nil {

@smacker smacker Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function has name startGitbaseWithClient but it doesn't start client anymore. Just gitbase.

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here and here.

smacker
smacker reviewed Apr 22, 2019
View reviewed changes
Comment thread docker/docker_exec.go Outdated
}

defer func() {
err = term.RestoreTerminal(fd, prevState)

@smacker smacker Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error is never returned from the function

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored this part here. It should be clearer now and the code is more decoupled.

smacker
smacker reviewed Apr 22, 2019
View reviewed changes
Comment thread docker/docker_exec.go Outdated
return &insResp, nil
}

func withRawTerminal(in io.ReadCloser, fn func() error) func() error {

@smacker smacker Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of this function and arguments below are a little confusing. withRawTerminal and rawTerminal. But actually you pass true and call this function even for non-terminal.

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answered here.

smacker
smacker reviewed Apr 22, 2019
View reviewed changes
Comment thread cmd/srcd/cmd/sql.go
err := docker.RemoveContainer(components.MysqlCli.Name)
if err != nil {
log.Warningf("could not stop mysql client: %v", err)
if insResp.ExitCode != 0 {

@smacker smacker Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we have tests for it. What happens when you exit with ctrl-c or ctrl-d? Does it exit correctly with exit code 0?

@se7entyse7en se7entyse7en Apr 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Now it exits with 0 on ctrl-d but not on ctrl-c. Gonna try adding a test for these.

@smacker smacker Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's okay to exit with error on ctrl-c in this case

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test for Ctrl-D here.

smacker
smacker reviewed Apr 23, 2019
View reviewed changes
Comment thread docker/docker_exec.go Outdated

return fn()
}
type stdioAttaccher struct {

@smacker smacker Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attach -> attacher

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here.

smacker
smacker reviewed Apr 23, 2019
View reviewed changes
Comment thread docker/docker_exec.go Outdated
// characters
prevState, err = term.SetRawTerminal(fd)
if err != nil {
done <- err

@smacker smacker Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have deadlock here

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮 eagle's eye! thanks for noticing

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here

smacker
smacker reviewed Apr 23, 2019
View reviewed changes
Comment thread docker/docker_exec.go
prevState, err = term.SetRawTerminal(fd)
if err != nil {
done <- err
go func() {

@smacker smacker Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's better to make the channel buffered?

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if in this case would slightly increase readability, I think that using an unbuffered channel keeps the code simpler compared to the readability gain of using a buffered channel.

@smacker smacker Apr 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly do you see as a downside of a buffered channel?

@se7entyse7en se7entyse7en Apr 23, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just an easier model to think 😄

@se7entyse7en
Updated Gitbase version to v0.20.0-beta2
2ca6e9b 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Added docker_exec.go with functions for executing commands in running…
cf1581d 
… containers

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Modified srcd sql command to use the mysql cli embedded in gitbase
08072c8 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Removed MysqlCli component
57c3767 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Removed references to MysqlCli component
baf3b29 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixed SQLREPLTestSuite
a8f3495 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixed components test
535e6ce 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixes in docker_exec.go
cf6f9f3 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Changed signature of Exec and ExecAttach to return status of exec pro…
8f602fc 
…cess

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Added check for exit code of MySQL cli in sql command
ea992d2 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Updatead gitbase to v0.20.0-beta3
4304ec3 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Refactored components start cmd
d3a5a0c 
I've separatead the starting of a component into a different function
so that it can be used by other commands as well.

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Reused startComponent function to start Gitbase component in sql command
9ae3c1c 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Refactored how stdio is attached
6508793 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Added test for interactive REPL ensuring correct exit with Ctrl-D
a495ab9 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixed typo in stdioAttacher name
15cd0be 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixed deadlock in withRawTerminal method
602fbe7 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Updated Gitbase version to v0.20.0-beta4
4db06e2 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Fixed TestSQLTestSuite/TestWrongQuery
0369669 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en

se7entyse7en commented Apr 26, 2019

Copy link
Copy Markdown
Contributor Author

Rebased on master and updated to use gitbase-v0.20.0-beta4.

@se7entyse7en
Update go.mod and vendor/modules.txt
352d0f2 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Update gitbase component to v0.20.0-rc1
1926b78 
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en

se7entyse7en commented May 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Updated to gitbase-v0.20.0-rc1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosms carlosms Awaiting requested review from carlosms
@dpordomingo dpordomingo Awaiting requested review from dpordomingo
1 more reviewer
@smacker smacker smacker left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update to gitbase 0.20 beta

3 participants

@se7entyse7en @carlosms @smacker