From 01655785b5c615070850f7a732c8013b3072ee61 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 10 Jul 2019 13:58:18 +0200 Subject: [PATCH 1/4] #43 Add support for JENKINS_OPTS, fix the --prefix bug --- .../jenkins/configuration/base/reconcile.go | 28 ++++++++++++++ .../configuration/base/reconcile_test.go | 38 +++++++++++++++++++ pkg/controller/jenkins/jenkins_controller.go | 6 +-- test/e2e/configuration_test.go | 2 +- 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 1efd89f8..024b85f3 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -127,6 +127,29 @@ func (r *ReconcileJenkinsBaseConfiguration) Reconcile() (reconcile.Result, jenki return result, jenkinsClient, err } +// GetJenkinsOpts put container JENKINS_OPTS env parameters in map and returns it +func GetJenkinsOpts(jenkins *v1alpha2.Jenkins) map[string]string { + envs := jenkins.Spec.Master.Containers[0].Env + jenkinsOpts := make(map[string]string) + + for k, v := range envs { + if v.Name == "JENKINS_OPTS" { + jenkinsOptsEnv := envs[k] + jenkinsOptsWithDashes := jenkinsOptsEnv.Value + jenkinsOptsWithDashes = strings.ReplaceAll(jenkinsOptsWithDashes, "--", "") // Remove dashes + jenkinsOptsWithEqOperators := strings.Split(jenkinsOptsWithDashes, " ") + + for _, vx := range jenkinsOptsWithEqOperators { + opt := strings.Split(vx, "=") + jenkinsOpts[opt[0]] = opt[1] + } + + return jenkinsOpts + } + } + return nil +} + func (r *ReconcileJenkinsBaseConfiguration) ensureResourcesRequiredForJenkinsPod(metaObject metav1.ObjectMeta) error { if err := r.createOperatorCredentialsSecret(metaObject); err != nil { return err @@ -748,6 +771,11 @@ func (r *ReconcileJenkinsBaseConfiguration) waitForJenkins(meta metav1.ObjectMet func (r *ReconcileJenkinsBaseConfiguration) ensureJenkinsClient(meta metav1.ObjectMeta) (jenkinsclient.Jenkins, error) { jenkinsURL, err := jenkinsclient.BuildJenkinsAPIUrl( r.jenkins.ObjectMeta.Namespace, resources.GetJenkinsHTTPServiceName(r.jenkins), r.jenkins.Spec.Service.Port, r.local, r.minikube) + + if prefix, ok := GetJenkinsOpts(r.jenkins)["prefix"]; ok { + jenkinsURL = jenkinsURL + prefix + } + if err != nil { return nil, err } diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index 15073db1..478f5b06 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -14,6 +14,44 @@ import ( corev1 "k8s.io/api/core/v1" ) +func TestGetJenkinsOpts(t *testing.T) { + envs := []corev1.EnvVar{ + {Name: "JENKINS_OPTS", Value: "--prefix=/jenkins --httpPort=8080"}, + } + + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: envs, + }, + }, + }, + }, + } + + opts := GetJenkinsOpts(jenkins) + + t.Run("equal env vars", func(t *testing.T) { + assert.Equal(t, opts["prefix"], "/jenkins") + assert.Equal(t, opts["httpPort"], "8080") + }) + + t.Run("not equal env vars", func(t *testing.T) { + assert.NotEqual(t, opts["prefix"], "/jenkins_not_equal") + assert.NotEqual(t, opts["httpPort"], "80808") + }) + + t.Run("should exists", func(t *testing.T) { + assert.Contains(t, opts, "httpPort") + }) + + t.Run("should exists", func(t *testing.T) { + assert.NotContains(t, opts, "should_not_exists") + }) +} + func TestCompareContainerVolumeMounts(t *testing.T) { t.Run("happy with service account", func(t *testing.T) { expectedContainer := corev1.Container{ diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index ea56a719..83e6ec92 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,8 +3,7 @@ package jenkins import ( "context" "fmt" - "reflect" - + "github.com/go-logr/logr" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" jenkinsclient "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/client" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base" @@ -16,8 +15,6 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" - - "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -27,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "reflect" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" diff --git a/test/e2e/configuration_test.go b/test/e2e/configuration_test.go index 44d641b8..a3d06361 100644 --- a/test/e2e/configuration_test.go +++ b/test/e2e/configuration_test.go @@ -167,7 +167,7 @@ func verifyJenkinsMasterPodAttributes(t *testing.T, jenkins *v1alpha2.Jenkins) { assert.Equal(t, resources.JenkinsMasterContainerName, jenkinsPod.Spec.Containers[0].Name) assert.Equal(t, len(jenkins.Spec.Master.Containers), len(jenkinsPod.Spec.Containers)) - + assert.Equal(t, jenkins.Spec.Master.SecurityContext, jenkinsPod.Spec.SecurityContext) assert.Equal(t, jenkins.Spec.Master.Containers[0].Command, jenkinsPod.Spec.Containers[0].Command) From 9e73be7a490a93d6c7f07b4f9503d61297fa0afc Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 10 Jul 2019 15:19:26 +0200 Subject: [PATCH 2/4] #43 Improve tests and fix GetJenkinsOpts --- .../jenkins/configuration/base/reconcile.go | 7 +- .../configuration/base/reconcile_test.go | 91 ++++++++++++++----- pkg/controller/jenkins/jenkins_controller.go | 6 +- 3 files changed, 75 insertions(+), 29 deletions(-) diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 024b85f3..1709e37d 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -136,12 +136,15 @@ func GetJenkinsOpts(jenkins *v1alpha2.Jenkins) map[string]string { if v.Name == "JENKINS_OPTS" { jenkinsOptsEnv := envs[k] jenkinsOptsWithDashes := jenkinsOptsEnv.Value - jenkinsOptsWithDashes = strings.ReplaceAll(jenkinsOptsWithDashes, "--", "") // Remove dashes + if len(jenkinsOptsWithDashes) == 0 { + return nil + } + jenkinsOptsWithEqOperators := strings.Split(jenkinsOptsWithDashes, " ") for _, vx := range jenkinsOptsWithEqOperators { opt := strings.Split(vx, "=") - jenkinsOpts[opt[0]] = opt[1] + jenkinsOpts[strings.ReplaceAll(opt[0], "--", "")] = opt[1] } return jenkinsOpts diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index 478f5b06..d8599f81 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -15,40 +15,81 @@ import ( ) func TestGetJenkinsOpts(t *testing.T) { - envs := []corev1.EnvVar{ - {Name: "JENKINS_OPTS", Value: "--prefix=/jenkins --httpPort=8080"}, - } - - jenkins := &v1alpha2.Jenkins{ - Spec: v1alpha2.JenkinsSpec{ - Master: v1alpha2.JenkinsMaster{ - Containers: []v1alpha2.Container{ - { - Env: envs, + t.Run("JENKINS_OPTS is empty", func(t *testing.T) { + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: []corev1.EnvVar{ + {Name: "JENKINS_OPTS", Value: ""}, + }, + }, }, }, }, - }, - } + } - opts := GetJenkinsOpts(jenkins) - - t.Run("equal env vars", func(t *testing.T) { - assert.Equal(t, opts["prefix"], "/jenkins") - assert.Equal(t, opts["httpPort"], "8080") + opts := GetJenkinsOpts(jenkins) + assert.Equal(t, 0, len(opts)) }) - t.Run("not equal env vars", func(t *testing.T) { - assert.NotEqual(t, opts["prefix"], "/jenkins_not_equal") - assert.NotEqual(t, opts["httpPort"], "80808") + t.Run("JENKINS_OPTS have --prefix argument ", func(t *testing.T) { + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: []corev1.EnvVar{ + {Name: "JENKINS_OPTS", Value: "--prefix=/jenkins"}, + }, + }, + }, + }, + }, + } + + opts := GetJenkinsOpts(jenkins) + + assert.Equal(t, 1, len(opts)) + + t.Run("ensure that JENKINS_OPTS not contains --httpPort", func(t *testing.T) { + assert.NotContains(t, opts, "httpPort") + }) + + t.Run("ensure that argument is --prefix", func(t *testing.T) { + assert.Contains(t, opts, "prefix") + }) }) - t.Run("should exists", func(t *testing.T) { - assert.Contains(t, opts, "httpPort") - }) + t.Run("JENKINS_OPTS have --prefix and --httpPort argument", func(t *testing.T) { + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: []corev1.EnvVar{ + {Name: "JENKINS_OPTS", Value: "--prefix=/jenkins --httpPort=8080"}, + }, + }, + }, + }, + }, + } - t.Run("should exists", func(t *testing.T) { - assert.NotContains(t, opts, "should_not_exists") + opts := GetJenkinsOpts(jenkins) + + assert.Equal(t, 2, len(opts)) + + t.Run("ensure that argument is --prefix with value /jenkins", func(t *testing.T) { + assert.Contains(t, opts, "prefix") + assert.Equal(t, opts["prefix"], "/jenkins") + }) + + t.Run("ensure that argument is --httpPort with value 8080", func(t *testing.T) { + assert.Contains(t, opts, "httpPort") + assert.Equal(t, opts["httpPort"], "8080") + }) }) } diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 83e6ec92..ea56a719 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,7 +3,8 @@ package jenkins import ( "context" "fmt" - "github.com/go-logr/logr" + "reflect" + "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" jenkinsclient "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/client" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base" @@ -15,6 +16,8 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" + + "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -24,7 +27,6 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" - "reflect" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" From 076b0aa453db76410c94172190162780a34309a4 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 10 Jul 2019 15:49:53 +0200 Subject: [PATCH 3/4] #43 Add additional test scenarios --- .../jenkins/configuration/base/reconcile.go | 2 +- .../configuration/base/reconcile_test.go | 65 ++++++++++++++----- 2 files changed, 50 insertions(+), 17 deletions(-) diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 1709e37d..7cb591a4 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -139,7 +139,7 @@ func GetJenkinsOpts(jenkins *v1alpha2.Jenkins) map[string]string { if len(jenkinsOptsWithDashes) == 0 { return nil } - + jenkinsOptsWithEqOperators := strings.Split(jenkinsOptsWithDashes, " ") for _, vx := range jenkinsOptsWithEqOperators { diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index d8599f81..80b313db 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -15,6 +15,25 @@ import ( ) func TestGetJenkinsOpts(t *testing.T) { + t.Run("JENKINS_OPTS is uninitialized", func(t *testing.T) { + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: []corev1.EnvVar{ + {Name: "", Value: ""}, + }, + }, + }, + }, + }, + } + + opts := GetJenkinsOpts(jenkins) + assert.Equal(t, 0, len(opts)) + }) + t.Run("JENKINS_OPTS is empty", func(t *testing.T) { jenkins := &v1alpha2.Jenkins{ Spec: v1alpha2.JenkinsSpec{ @@ -52,14 +71,9 @@ func TestGetJenkinsOpts(t *testing.T) { opts := GetJenkinsOpts(jenkins) assert.Equal(t, 1, len(opts)) - - t.Run("ensure that JENKINS_OPTS not contains --httpPort", func(t *testing.T) { - assert.NotContains(t, opts, "httpPort") - }) - - t.Run("ensure that argument is --prefix", func(t *testing.T) { - assert.Contains(t, opts, "prefix") - }) + assert.NotContains(t, opts, "httpPort") + assert.Contains(t, opts, "prefix") + assert.Equal(t, opts["prefix"], "/jenkins") }) t.Run("JENKINS_OPTS have --prefix and --httpPort argument", func(t *testing.T) { @@ -81,15 +95,34 @@ func TestGetJenkinsOpts(t *testing.T) { assert.Equal(t, 2, len(opts)) - t.Run("ensure that argument is --prefix with value /jenkins", func(t *testing.T) { - assert.Contains(t, opts, "prefix") - assert.Equal(t, opts["prefix"], "/jenkins") - }) + assert.Contains(t, opts, "prefix") + assert.Equal(t, opts["prefix"], "/jenkins") - t.Run("ensure that argument is --httpPort with value 8080", func(t *testing.T) { - assert.Contains(t, opts, "httpPort") - assert.Equal(t, opts["httpPort"], "8080") - }) + assert.Contains(t, opts, "httpPort") + assert.Equal(t, opts["httpPort"], "8080") + }) + + t.Run("JENKINS_OPTS have --httpPort argument", func(t *testing.T) { + jenkins := &v1alpha2.Jenkins{ + Spec: v1alpha2.JenkinsSpec{ + Master: v1alpha2.JenkinsMaster{ + Containers: []v1alpha2.Container{ + { + Env: []corev1.EnvVar{ + {Name: "JENKINS_OPTS", Value: "--httpPort=8080"}, + }, + }, + }, + }, + }, + } + + opts := GetJenkinsOpts(jenkins) + + assert.Equal(t, 2, len(opts)) + assert.NotContains(t, opts, "prefix") + assert.Contains(t, opts, "httpPort") + assert.Equal(t, opts["httpPort"], "8080") }) } From 164fe30aef4e7680caee3b485f8d5cd351b340f1 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 10 Jul 2019 16:08:09 +0200 Subject: [PATCH 4/4] #43 Fix tests --- pkg/controller/jenkins/configuration/base/reconcile_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index 80b313db..82386155 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -119,7 +119,7 @@ func TestGetJenkinsOpts(t *testing.T) { opts := GetJenkinsOpts(jenkins) - assert.Equal(t, 2, len(opts)) + assert.Equal(t, 1, len(opts)) assert.NotContains(t, opts, "prefix") assert.Contains(t, opts, "httpPort") assert.Equal(t, opts["httpPort"], "8080")