Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Update to Gitbase v0.20.0 #439

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

[WIP] Update to Gitbase v0.20.0 #439

wants to merge 21 commits into from

Conversation

se7entyse7en
Copy link
Contributor

@se7entyse7en se7entyse7en commented Apr 12, 2019 •
edited

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
Copy link
Contributor Author

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

@carlosms
Copy link
Contributor

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

@se7entyse7en
Copy link
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
cmd/srcd/cmd/sql.go
@@ -192,81 +162,32 @@ func startGitbaseWithClient(client api.EngineClient) error {
return humanizef(err, "could not start gitbase")
}

if err := docker.EnsureInstalled(components.MysqlCli.Image, components.MysqlCli.Version); err != nil {
Copy link
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.

Copy link
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
docker/docker_exec.go Outdated
}

defer func() {
err = term.RestoreTerminal(fd, prevState)
Copy link
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

Copy link
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
docker/docker_exec.go Outdated
return &insResp, nil
}

func withRawTerminal(in io.ReadCloser, fn func() error) func() error {
Copy link
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.

Copy link
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
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 {
Copy link
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?

Copy link
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.

Copy link
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

Copy link
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
docker/docker_exec.go Outdated

return fn()
}
type stdioAttaccher struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

attach -> attacher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
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
docker/docker_exec.go Outdated
// characters
prevState, err = term.SetRawTerminal(fd)
if err != nil {
done <- err
Copy link
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

Copy link
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

Copy link
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
docker/docker_exec.go
@@ -155,7 +155,10 @@ func (sa *stdioAttacher) withRawTerminal(fn attachFn) <-chan error {
// characters
prevState, err = term.SetRawTerminal(fd)
if err != nil {
done <- err
go func() {
Copy link
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?

Copy link
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.

Copy link
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?

Copy link
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
Copy link
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
Copy link
Contributor Author

se7entyse7en commented May 1, 2019 •
edited

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

@smacker smacker smacker left review comments

@carlosms carlosms Awaiting requested review from carlosms

@dpordomingo dpordomingo Awaiting requested review from dpordomingo

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