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.

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

Fix git SSH clone in docker container #172

Merged
merged 10 commits into from Feb 23, 2019

Conversation

michelvocks
Copy link
Member

Fixes #149

@Skarlso Feel free to merge if it's okay from your side. 🤗

workers/pipeline/git.go Outdated Show resolved Hide resolved
@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

@michelvocks @leberknecht Alright. What do you guys think? We are checking the host key, but if there is a problem with it, we move on but Log a Warning error message.

In order for this to be visible on the UI though, we would need to return the error, which stops the flow right there. Which is not what we want if I understand correctly.

@codecov-io
Copy link

codecov-io commented Feb 20, 2019

Codecov Report

Merging #172 into master will increase coverage by 0.25%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   66.52%   66.78%   +0.25%     
==========================================
  Files          33       33              
  Lines        2593     2628      +35     
==========================================
+ Hits         1725     1755      +30     
+ Misses        654      650       -4     
- Partials      214      223       +9
Impacted Files Coverage Δ
workers/pipeline/git.go 72.43% <62.5%> (+3.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6987967...59d1680. Read the comment docs.

@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

I'll add a test soon... daughter woke up.. :)

@michelvocks
Copy link
Member Author

michelvocks commented Feb 20, 2019

This is perfect in my opinion. Thanks @Skarlso 🤗

I gave it a try in a docker container but it fails with the following panic:

2019-02-20T14:16:45.766Z [ERROR] Gaia: Warning: Unknown host key.
{"time":"2019-02-20T14:16:45.7667359Z","level":"-","prefix":"echo","file":"recover.go","line":"73","message":"[PANIC RECOVER] runtime error: invalid memory address or nil pointer dereference goroutine 41 [running]:
github.com/labstack/echo/middleware.RecoverWithConfig.func1.1.1(0x21e47c8, 0x1000, 0xc000500000, 0x229d240, 0xc0001ce000)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+incompatible/middleware/recover.go:71 +0xf8
panic(0xc1eaa0, 0x285d5d0)
	/usr/local/Cellar/go/1.11.4/libexec/src/runtime/panic.go:513 +0x1b9
gopkg.in/src-d/go-git.v4/plumbing/transport/internal/common.(*session).finish(0x0, 0x0, 0x0)
	/Users/michelvocks/go/pkg/mod/gopkg.in/src-d/go-git.v4@v4.5.0/plumbing/transport/internal/common/common.go:334 +0x26
gopkg.in/src-d/go-git.v4/plumbing/transport/internal/common.(*session).Close(0x0, 0x0, 0x0)
	/Users/michelvocks/go/pkg/mod/gopkg.in/src-d/go-git.v4@v4.5.0/plumbing/transport/internal/common/common.go:352 +0x37
panic(0xc1eaa0, 0x285d5d0)
	/usr/local/Cellar/go/1.11.4/libexec/src/runtime/panic.go:513 +0x1b9
gopkg.in/src-d/go-git.v4/plumbing/transport/internal/common.(*session).AdvertisedReferences(0x0, 0x228fd20, 0x0, 0x0)
	/Users/michelvocks/go/pkg/mod/gopkg.in/src-d/go-git.v4@v4.5.0/plumbing/transport/internal/common/common.go:167 +0x26
github.com/gaia-pipeline/gaia/workers/pipeline.GitLSRemote(0xc0002a6aa0, 0x0, 0x0)
	/Users/michelvocks/go/src/github.com/gaia-pipeline/gaia/workers/pipeline/git.go:71 +0x20b
github.com/gaia-pipeline/gaia/handlers.PipelineGitLSRemote(0x229d240, 0xc0001ce000, 0xc0004fc498, 0x0)
	/Users/michelvocks/go/src/github.com/gaia-pipeline/gaia/handlers/pipeline.go:27 +0x81
github.com/labstack/echo.(*Echo).Add.func1(0x229d240, 0xc0001ce000, 0xc0001acc60, 0xc0000b2f60)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+incompatible/echo.go:490 +0x8a
github.com/gaia-pipeline/gaia/handlers.AuthMiddleware.func1.1(0x229d240, 0xc0001ce000, 0xc00000a140, 0xc3f180)
	/Users/michelvocks/go/src/github.com/gaia-pipeline/gaia/handlers/auth.go:54 +0x408
github.com/labstack/echo/middleware.BodyLimitWithConfig.func1.1(0x229d240, 0xc0001ce000, 0x0, 0x0)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+incompatible/middleware/body_limit.go:87 +0x187
github.com/labstack/echo/middleware.RecoverWithConfig.func1.1(0x229d240, 0xc0001ce000, 0x0, 0x0)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+incompatible/middleware/recover.go:78 +0xd8
github.com/labstack/echo.(*Echo).ServeHTTP(0xc00040c000, 0x2290280, 0xc00021fa40, 0xc0001dfc00)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+incompatible/echo.go:593 +0x220
net/http.serverHandler.ServeHTTP(0xc000098680, 0x2290280, 0xc00021fa40, 0xc0001dfc00)
	/usr/local/Cellar/go/1.11.4/libexec/src/net/http/server.go:2741 +0xab
net/http.(*conn).serve(0xc0002a60a0, 0x22910c0, 0xc0002ba040)
	/usr/local/Cellar/go/1.11.4/libexec/src/net/http/server.go:1847 +0x646
created by net/http.(*Server).Serve
	/usr/local/Cellar/go/1.11.4/libexec/src/net/http/server.go:2851 +0x2f5

goroutine 1 [IO wait, 2 minutes]:
internal/poll.runtime_pollWait(0x7f950630df00, 0x72, 0x0)
	/usr/local/Cellar/go/1.11.4/libexec/src/runtime/netpoll.go:173 +0x66
internal/poll.(*pollDesc).wait(0xc00020e718, 0x72, 0xc0001e0000, 0x0, 0x0)
	/usr/local/Cellar/go/1.11.4/libexec/src/internal/poll/fd_poll_runtime.go:85 +0x9a
internal/poll.(*pollDesc).waitRead(0xc00020e718, 0xffffffffffffff00, 0x0, 0x0)
	/usr/local/Cellar/go/1.11.4/libexec/src/internal/poll/fd_poll_runtime.go:90 +0x3d
internal/poll.(*FD).Accept(0xc00020e700, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.11.4/libexec/src/internal/poll/fd_unix.go:384 +0x1a0
net.(*netFD).accept(0xc00020e700, 0x203000, 0x0, 0x0)
	/usr/local/Cellar/go/1.11.4/libexec/src/net/fd_unix.go:238 +0x42
net.(*TCPListener).accept(0xc0001ea2d0, 0xc000044f00, 0x40b77f, 0xc0002f80a0)
	/usr/local/Cellar/go/1.11.4/libexec/src/net/tcpsock_posix.go:139 +0x2e
net.(*TCPListener).AcceptTCP(0xc0001ea2d0, 0xfba3a5d3, 0x89ad5208cca40317, 0x4491b8)
	/usr/local/Cellar/go/1.11.4/libexec/src/net/tcpsock.go:247 +0x47
github.com/labstack/echo.tcpKeepAliveListener.Accept(0xc0001ea2d0, 0xc0001f7b50, 0x47b846, 0x5c6d60d5, 0x433fce)
	/Users/michelvocks/go/pkg/mod/github.com/labstack/echo@v3.3.10+in
"}

I think the problem is that s, err := cl.NewUploadPackSession(ep, auth) returns an error and s will be nil.

@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

@michelvocks intersting

@Skarlso Skarlso closed this Feb 20, 2019
@Skarlso Skarlso reopened this Feb 20, 2019
@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

Ups... So... Interesting. Thanks! I'll check it out and fix it. 😊

@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

Ah, I see. I thought it would just ignore missing files. But it doesn't. :) Fixing it. :)

@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

Oh I see what you mean...

@Skarlso
Copy link
Member

Skarlso commented Feb 20, 2019

@michelvocks It was worse than that. :D I was checking in the wrong place. Basically, since the operation failed with an invalid host key.. I have to retry said operation again with an Auth that is using an Ignore.

It looks a bit more verbose this way though, check it out. I'll push in a bit.

@michelvocks
Copy link
Member Author

It works now @Skarlso. I did some minor changes to make the logging a bit more verbose and added an extra small check.

@Skarlso
Copy link
Member

Skarlso commented Feb 21, 2019

@michelvocks looks good. I'm unsure yet how I'll test this as it finds my local known_hosts file. I'll try to mock the hostname from where it will clone. Maybe that'll work.

@Skarlso Skarlso added Needs Work The PR still requires some work from the submitter. and removed Ready To Merge PR is ready to be merged into master labels Feb 21, 2019
@Skarlso
Copy link
Member

Skarlso commented Feb 21, 2019

Note: Working on writing / fixing test for GitLS

@Skarlso
Copy link
Member

Skarlso commented Feb 21, 2019

@michelvocks first passing test!!! :)))))))

@Skarlso
Copy link
Member

Skarlso commented Feb 22, 2019

@michelvocks alright. Good as it gets. decrease by 0.2%. :D

@Skarlso Skarlso added Ready To Merge PR is ready to be merged into master and removed Needs Work The PR still requires some work from the submitter. labels Feb 22, 2019
@Skarlso
Copy link
Member

Skarlso commented Feb 23, 2019

Yay, I managed to increase coverage. :)

@michelvocks
Copy link
Member Author

Thanks @Skarlso. Awesome job! 🤗

@michelvocks michelvocks merged commit 3f5c4c3 into gaia-pipeline:master Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready To Merge PR is ready to be merged into master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants