Commit 0009e06f authored by Tomasz Maczukin's avatar Tomasz Maczukin 🌴

Merge branch 'properly-support-feature-set-for-shells' into 'master'

Fix support for features for shells

See merge request gitlab-org/gitlab-runner!989
parents 48074b9f 5105b237
Pipeline #27383601 passed with stages
in 27 minutes and 56 seconds
......@@ -83,8 +83,11 @@ func mockingExecutionStack(t *testing.T, executorName string, maxBuilds int, job
}
//ExecutorProvider
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Times(maxBuilds + 1)
p.On("Create").Return(&e).Times(maxBuilds)
p.On("GetFeatures", mock.Anything).Times(maxBuilds)
p.On("Acquire", mock.Anything).Return(&common.MockExecutorData{}, nil).Times(maxBuilds)
p.On("Release", mock.Anything, mock.Anything).Return(nil).Times(maxBuilds)
......
......@@ -26,8 +26,11 @@ func TestBuildRun(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor only once
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// We run everything once
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
......@@ -71,8 +74,11 @@ func TestRetryPrepare(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Times(3)
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).
......@@ -112,8 +118,11 @@ func TestPrepareFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Times(3)
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).
......@@ -144,8 +153,11 @@ func TestPrepareFailureOnBuildError(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Times(1)
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).
......@@ -182,8 +194,11 @@ func TestRunFailureRunsAfterScriptAndArtifactsOnFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -224,8 +239,11 @@ func TestGetSourcesRunFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -264,8 +282,11 @@ func TestArtifactDownloadRunFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -306,8 +327,11 @@ func TestArtifactUploadRunFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -357,8 +381,11 @@ func TestRestoreCacheRunFailure(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -397,8 +424,11 @@ func TestRunWrongAttempts(t *testing.T) {
defer p.AssertExpectations(t)
// Create executor
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e)
p.On("GetFeatures", mock.Anything).Once()
// Prepare plan
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil)
......@@ -433,8 +463,11 @@ func TestRunSuccessOnSecondAttempt(t *testing.T) {
p := MockExecutorProvider{}
// Create executor only once
p.On("CanCreate").Return(true).Once()
p.On("GetDefaultShell").Return("bash").Once()
p.On("GetFeatures", mock.Anything).Return(nil).Twice()
p.On("Create").Return(&e).Once()
p.On("GetFeatures", mock.Anything).Once()
// We run everything once
e.On("Prepare", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
......
......@@ -2,6 +2,8 @@ package common
import (
"context"
"errors"
"fmt"
log "github.com/sirupsen/logrus"
)
......@@ -47,7 +49,8 @@ type ExecutorProvider interface {
Create() Executor
Acquire(config *RunnerConfig) (ExecutorData, error)
Release(config *RunnerConfig, data ExecutorData) error
GetFeatures(features *FeaturesInfo)
GetFeatures(features *FeaturesInfo) error
GetDefaultShell() string
}
type BuildError struct {
......@@ -64,9 +67,29 @@ func (b *BuildError) Error() string {
var executors map[string]ExecutorProvider
func validateExecutorProvider(provider ExecutorProvider) error {
if provider.GetDefaultShell() == "" {
return errors.New("default shell not implemented")
}
if !provider.CanCreate() {
return errors.New("cannot create executor")
}
if err := provider.GetFeatures(&FeaturesInfo{}); err != nil {
return fmt.Errorf("cannot get features: %v", err)
}
return nil
}
func RegisterExecutor(executor string, provider ExecutorProvider) {
log.Debugln("Registering", executor, "executor...")
if err := validateExecutorProvider(provider); err != nil {
panic("Executor cannot be registered: " + err.Error())
}
if executors == nil {
executors = make(map[string]ExecutorProvider)
}
......
......@@ -64,9 +64,32 @@ func (_m *MockExecutorProvider) Create() Executor {
return r0
}
// GetDefaultShell provides a mock function with given fields:
func (_m *MockExecutorProvider) GetDefaultShell() string {
ret := _m.Called()
var r0 string
if rf, ok := ret.Get(0).(func() string); ok {
r0 = rf()
} else {
r0 = ret.Get(0).(string)
}
return r0
}
// GetFeatures provides a mock function with given fields: features
func (_m *MockExecutorProvider) GetFeatures(features *FeaturesInfo) {
_m.Called(features)
func (_m *MockExecutorProvider) GetFeatures(features *FeaturesInfo) error {
ret := _m.Called(features)
var r0 error
if rf, ok := ret.Get(0).(func(*FeaturesInfo) error); ok {
r0 = rf(features)
} else {
r0 = ret.Error(0)
}
return r0
}
// Release provides a mock function with given fields: config, data
......
......@@ -93,6 +93,7 @@ type VersionInfo struct {
Platform string `json:"platform,omitempty"`
Architecture string `json:"architecture,omitempty"`
Executor string `json:"executor,omitempty"`
Shell string `json:"shell,omitempty"`
Features FeaturesInfo `json:"features"`
}
......
package executors
import "gitlab.com/gitlab-org/gitlab-runner/common"
import (
"errors"
"gitlab.com/gitlab-org/gitlab-runner/common"
)
type DefaultExecutorProvider struct {
Creator func() common.Executor
FeaturesUpdater func(features *common.FeaturesInfo)
Creator func() common.Executor
FeaturesUpdater func(features *common.FeaturesInfo)
DefaultShellName string
}
func (e DefaultExecutorProvider) CanCreate() bool {
......@@ -26,8 +31,15 @@ func (e DefaultExecutorProvider) Release(config *common.RunnerConfig, data commo
return nil
}
func (e DefaultExecutorProvider) GetFeatures(features *common.FeaturesInfo) {
if e.FeaturesUpdater != nil {
e.FeaturesUpdater(features)
func (e DefaultExecutorProvider) GetFeatures(features *common.FeaturesInfo) error {
if e.FeaturesUpdater == nil {
return errors.New("cannot evaluate features")
}
e.FeaturesUpdater(features)
return nil
}
func (e DefaultExecutorProvider) GetDefaultShell() string {
return e.DefaultShellName
}
......@@ -121,7 +121,8 @@ func init() {
}
common.RegisterExecutor("docker", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -112,7 +112,8 @@ func init() {
}
common.RegisterExecutor("docker-ssh", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -405,8 +405,12 @@ func (m *machineProvider) CanCreate() bool {
return m.provider.CanCreate()
}
func (m *machineProvider) GetFeatures(features *common.FeaturesInfo) {
m.provider.GetFeatures(features)
func (m *machineProvider) GetFeatures(features *common.FeaturesInfo) error {
return m.provider.GetFeatures(features)
}
func (m *machineProvider) GetDefaultShell() string {
return m.provider.GetDefaultShell()
}
func (m *machineProvider) Create() common.Executor {
......
......@@ -549,7 +549,8 @@ func featuresFn(features *common.FeaturesInfo) {
func init() {
common.RegisterExecutor("kubernetes", executors.DefaultExecutorProvider{
Creator: createFn,
FeaturesUpdater: featuresFn,
Creator: createFn,
FeaturesUpdater: featuresFn,
DefaultShellName: executorOptions.Shell.Shell,
})
}
......@@ -342,7 +342,8 @@ func init() {
}
common.RegisterExecutor("parallels", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -9,12 +9,13 @@ import (
"path/filepath"
"fmt"
"time"
"github.com/kardianos/osext"
"github.com/sirupsen/logrus"
"gitlab.com/gitlab-org/gitlab-runner/common"
"gitlab.com/gitlab-org/gitlab-runner/executors"
"gitlab.com/gitlab-org/gitlab-runner/helpers"
"time"
)
type executor struct {
......@@ -158,7 +159,8 @@ func init() {
}
common.RegisterExecutor("shell", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -88,7 +88,8 @@ func init() {
}
common.RegisterExecutor("ssh", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -3,11 +3,12 @@ package virtualbox
import (
"errors"
"fmt"
"time"
"gitlab.com/gitlab-org/gitlab-runner/common"
"gitlab.com/gitlab-org/gitlab-runner/executors"
"gitlab.com/gitlab-org/gitlab-runner/helpers/ssh"
vbox "gitlab.com/gitlab-org/gitlab-runner/helpers/virtualbox"
"time"
)
type executor struct {
......@@ -321,7 +322,8 @@ func init() {
}
common.RegisterExecutor("virtualbox", executors.DefaultExecutorProvider{
Creator: creator,
FeaturesUpdater: featuresUpdater,
Creator: creator,
FeaturesUpdater: featuresUpdater,
DefaultShellName: options.Shell.Shell,
})
}
......@@ -132,13 +132,18 @@ func (n *GitLabClient) getRunnerVersion(config common.RunnerConfig) common.Versi
Platform: runtime.GOOS,
Architecture: runtime.GOARCH,
Executor: config.Executor,
Shell: config.Shell,
}
if executor := common.GetExecutor(config.Executor); executor != nil {
executor.GetFeatures(&info.Features)
if info.Shell == "" {
info.Shell = executor.GetDefaultShell()
}
}
if shell := common.GetShell(config.Shell); shell != nil {
if shell := common.GetShell(info.Shell); shell != nil {
shell.GetFeatures(&info.Features)
}
......
......@@ -13,6 +13,7 @@ import (
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
. "gitlab.com/gitlab-org/gitlab-runner/common"
......@@ -982,3 +983,56 @@ func TestArtifactsDownload(t *testing.T) {
state = c.DownloadArtifacts(fileNotFoundTokenCredentials, artifactsFileName)
assert.Equal(t, DownloadNotFound, state, "Artifacts should be bit downloaded if it's not found")
}
func TestRunnerVersion(t *testing.T) {
c := NewGitLabClient()
info := c.getRunnerVersion(RunnerConfig{
RunnerSettings: RunnerSettings{
Executor: "my-executor",
Shell: "my-shell",
},
})
assert.NotEmpty(t, info.Name)
assert.NotEmpty(t, info.Version)
assert.NotEmpty(t, info.Revision)
assert.NotEmpty(t, info.Platform)
assert.NotEmpty(t, info.Architecture)
assert.Equal(t, "my-executor", info.Executor)
assert.Equal(t, "my-shell", info.Shell)
}
func TestRunnerVersionToGetExecutorAndShellFeaturesWithTheDefaultShell(t *testing.T) {
executorProvider := MockExecutorProvider{}
defer executorProvider.AssertExpectations(t)
executorProvider.On("GetDefaultShell").Return("my-default-executor-shell").Twice()
executorProvider.On("CanCreate").Return(true).Once()
executorProvider.On("GetFeatures", mock.Anything).Return(nil).Run(func(args mock.Arguments) {
features := args[0].(*FeaturesInfo)
features.Shared = true
})
RegisterExecutor("my-test-executor", &executorProvider)
shell := MockShell{}
defer shell.AssertExpectations(t)
shell.On("GetName").Return("my-default-executor-shell")
shell.On("GetFeatures", mock.Anything).Return(nil).Run(func(args mock.Arguments) {
features := args[0].(*FeaturesInfo)
features.Variables = true
})
RegisterShell(&shell)
c := NewGitLabClient()
info := c.getRunnerVersion(RunnerConfig{
RunnerSettings: RunnerSettings{
Executor: "my-test-executor",
Shell: "",
},
})
assert.Equal(t, "my-test-executor", info.Executor)
assert.Equal(t, "my-default-executor-shell", info.Shell)
assert.False(t, info.Features.Artifacts, "dry-run that this is not enabled")
assert.True(t, info.Features.Shared, "feature is enabled by executor")
assert.True(t, info.Features.Variables, "feature is enabled by shell")
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment