Skip to content
Snippets Groups Projects
Unverified Commit 11d02d05 authored by Steve Azzopardi's avatar Steve Azzopardi
Browse files

Return specific err for executorCommand.Connect

Instead of returning a simple error string, return an specific error
type to help with type checking.

Add missing test cases.
parent 58bdddcb
No related branches found
No related tags found
No related merge requests found
...@@ -8,13 +8,22 @@ import ( ...@@ -8,13 +8,22 @@ import (
"time" "time"
"github.com/docker/docker/api/types" "github.com/docker/docker/api/types"
"gitlab.com/gitlab-org/gitlab-terminal"
"gitlab.com/gitlab-org/gitlab-runner/common" "gitlab.com/gitlab-org/gitlab-runner/common"
"gitlab.com/gitlab-org/gitlab-runner/helpers/docker" "gitlab.com/gitlab-org/gitlab-runner/helpers/docker"
terminalsession "gitlab.com/gitlab-org/gitlab-runner/session/terminal" terminalsession "gitlab.com/gitlab-org/gitlab-runner/session/terminal"
"gitlab.com/gitlab-org/gitlab-terminal"
) )
// buildContainerTerminalTimeout is the error used when the build container is
// not running yet an we have a terminal request waiting for the container to
// start and a certain amount of time is exceeded.
type buildContainerTerminalTimeout struct {
}
func (buildContainerTerminalTimeout) Error() string {
return "timeout for waiting for build container"
}
func (s *commandExecutor) watchForRunningBuildContainer(deadline time.Time) (string, error) { func (s *commandExecutor) watchForRunningBuildContainer(deadline time.Time) (string, error) {
for time.Since(deadline) < 0 { for time.Since(deadline) < 0 {
if s.buildContainer == nil { if s.buildContainer == nil {
...@@ -33,7 +42,7 @@ func (s *commandExecutor) watchForRunningBuildContainer(deadline time.Time) (str ...@@ -33,7 +42,7 @@ func (s *commandExecutor) watchForRunningBuildContainer(deadline time.Time) (str
} }
} }
return "", errors.New("timeout for waiting for build container") return "", buildContainerTerminalTimeout{}
} }
func (s *commandExecutor) Connect() (terminalsession.Conn, error) { func (s *commandExecutor) Connect() (terminalsession.Conn, error) {
...@@ -45,7 +54,7 @@ func (s *commandExecutor) Connect() (terminalsession.Conn, error) { ...@@ -45,7 +54,7 @@ func (s *commandExecutor) Connect() (terminalsession.Conn, error) {
// `gitlab-terminal` package. There are plans to improve this please take a // `gitlab-terminal` package. There are plans to improve this please take a
// look at https://gitlab.com/gitlab-org/gitlab-ce/issues/50384#proposal and // look at https://gitlab.com/gitlab-org/gitlab-ce/issues/50384#proposal and
// https://gitlab.com/gitlab-org/gitlab-terminal/issues/4 // https://gitlab.com/gitlab-org/gitlab-terminal/issues/4
containerID, err := s.watchForRunningBuildContainer(time.Now().Add(waitForContainerTimeout)) containerID, err := s.watchForRunningBuildContainer(time.Now().Add(time.Second))
if err != nil { if err != nil {
return nil, err return nil, err
} }
......
...@@ -133,14 +133,34 @@ func TestCommandExecutor_Connect(t *testing.T) { ...@@ -133,14 +133,34 @@ func TestCommandExecutor_Connect(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
buildContainerRunning bool buildContainerRunning bool
hasBuildContainer bool
containerInspectErr error
expectedErr error
}{ }{
{ {
name: "Connect Timeout", name: "Connect Timeout",
buildContainerRunning: false, buildContainerRunning: false,
hasBuildContainer: true,
expectedErr: buildContainerTerminalTimeout{},
}, },
{ {
name: "Successful connect", name: "Successful connect",
buildContainerRunning: true, buildContainerRunning: true,
hasBuildContainer: true,
containerInspectErr: nil,
},
{
name: "Container inspect failed",
buildContainerRunning: false,
hasBuildContainer: true,
containerInspectErr: errors.New("container not found"),
expectedErr: errors.New("container not found"),
},
{
name: "Not build container",
buildContainerRunning: false,
hasBuildContainer: false,
expectedErr: buildContainerTerminalTimeout{},
}, },
} }
...@@ -158,19 +178,23 @@ func TestCommandExecutor_Connect(t *testing.T) { ...@@ -158,19 +178,23 @@ func TestCommandExecutor_Connect(t *testing.T) {
}, },
client: c, client: c,
}, },
buildContainer: &types.ContainerJSON{ }
if test.hasBuildContainer {
s.buildContainer = &types.ContainerJSON{
ContainerJSONBase: &types.ContainerJSONBase{ ContainerJSONBase: &types.ContainerJSONBase{
ID: "1234", ID: "1234",
}, },
}, }
} }
c.On("ContainerInspect", s.Context, "1234").Return(types.ContainerJSON{ c.On("ContainerInspect", s.Context, "1234").Return(types.ContainerJSON{
ContainerJSONBase: &types.ContainerJSONBase{ ContainerJSONBase: &types.ContainerJSONBase{
State: &types.ContainerState{ State: &types.ContainerState{
Running: test.buildContainerRunning, Running: test.buildContainerRunning,
}, },
}, },
}, nil) }, test.containerInspectErr)
conn, err := s.Connect() conn, err := s.Connect()
...@@ -181,7 +205,7 @@ func TestCommandExecutor_Connect(t *testing.T) { ...@@ -181,7 +205,7 @@ func TestCommandExecutor_Connect(t *testing.T) {
return return
} }
assert.EqualError(t, err, "timeout for waiting for build container") assert.EqualError(t, err, test.expectedErr.Error())
assert.Nil(t, conn) assert.Nil(t, conn)
}) })
} }
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment