From da21f4ce7e7e6b6c4b01189af42ba0ed50a7ea35 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 25 Sep 2019 16:27:50 +0200 Subject: [PATCH 01/16] Update getting-started.md --- documentation/v0.2.0/getting-started.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/v0.2.0/getting-started.md b/documentation/v0.2.0/getting-started.md index 390ecc3b..cf152856 100644 --- a/documentation/v0.2.0/getting-started.md +++ b/documentation/v0.2.0/getting-started.md @@ -144,7 +144,7 @@ pipelineJob('build-jenkins-operator') { } ``` -**cicd/jobs/build.jenkins** it's an actual Jenkins pipeline: +**cicd/pipelines/build.jenkins** it's an actual Jenkins pipeline: ``` #!/usr/bin/env groovy From 7ac29e543b68e28a2d94fcf95c160eef60e3632d Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 25 Sep 2019 16:28:47 +0200 Subject: [PATCH 02/16] Update getting-started.md --- documentation/v0.1.1/getting-started.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/v0.1.1/getting-started.md b/documentation/v0.1.1/getting-started.md index 0f6ca8db..56a79ccc 100644 --- a/documentation/v0.1.1/getting-started.md +++ b/documentation/v0.1.1/getting-started.md @@ -142,7 +142,7 @@ pipelineJob('build-jenkins-operator') { } ``` -**cicd/jobs/build.jenkins** it's an actual Jenkins pipeline: +**cicd/pipelines/build.jenkins** it's an actual Jenkins pipeline: ``` #!/usr/bin/env groovy From 22b8bcedf7a3b94b97ca4ca2632e1ec883afcf3c Mon Sep 17 00:00:00 2001 From: Dmitry Stoyanov Date: Fri, 27 Sep 2019 10:42:50 +0300 Subject: [PATCH 03/16] Fix typo in namespace --- documentation/v0.1.1/getting-started.md | 4 ++-- documentation/v0.2.0/getting-started.md | 4 ++-- .../Getting Started/v0.1.1/configure-backup-and-restore.md | 4 ++-- .../Getting Started/v0.2.0/configure-backup-and-restore.md | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/documentation/v0.1.1/getting-started.md b/documentation/v0.1.1/getting-started.md index 56a79ccc..56ae587d 100644 --- a/documentation/v0.1.1/getting-started.md +++ b/documentation/v0.1.1/getting-started.md @@ -418,7 +418,7 @@ apiVersion: v1 kind: PersistentVolumeClaim metadata: name: - namespace: + namespace: spec: accessModes: - ReadWriteOnce @@ -429,7 +429,7 @@ spec: Run command: ```bash -$ kubectl -n create -f pvc.yaml +$ kubectl -n create -f pvc.yaml ``` #### Configure Jenkins CR diff --git a/documentation/v0.2.0/getting-started.md b/documentation/v0.2.0/getting-started.md index cf152856..17ac49e9 100644 --- a/documentation/v0.2.0/getting-started.md +++ b/documentation/v0.2.0/getting-started.md @@ -563,7 +563,7 @@ apiVersion: v1 kind: PersistentVolumeClaim metadata: name: - namespace: + namespace: spec: accessModes: - ReadWriteOnce @@ -574,7 +574,7 @@ spec: Run command: ```bash -$ kubectl -n create -f pvc.yaml +$ kubectl -n create -f pvc.yaml ``` #### Configure Jenkins CR diff --git a/website/content/en/docs/Getting Started/v0.1.1/configure-backup-and-restore.md b/website/content/en/docs/Getting Started/v0.1.1/configure-backup-and-restore.md index 194f8c0a..87740176 100644 --- a/website/content/en/docs/Getting Started/v0.1.1/configure-backup-and-restore.md +++ b/website/content/en/docs/Getting Started/v0.1.1/configure-backup-and-restore.md @@ -17,7 +17,7 @@ apiVersion: v1 kind: PersistentVolumeClaim metadata: name: - namespace: + namespace: spec: accessModes: - ReadWriteOnce @@ -28,7 +28,7 @@ spec: Run command: ```bash -$ kubectl -n create -f pvc.yaml +$ kubectl -n create -f pvc.yaml ``` #### Configure Jenkins CR diff --git a/website/content/en/docs/Getting Started/v0.2.0/configure-backup-and-restore.md b/website/content/en/docs/Getting Started/v0.2.0/configure-backup-and-restore.md index 3686aa99..fe279dfe 100644 --- a/website/content/en/docs/Getting Started/v0.2.0/configure-backup-and-restore.md +++ b/website/content/en/docs/Getting Started/v0.2.0/configure-backup-and-restore.md @@ -19,7 +19,7 @@ apiVersion: v1 kind: PersistentVolumeClaim metadata: name: - namespace: + namespace: spec: accessModes: - ReadWriteOnce @@ -30,7 +30,7 @@ spec: Run command: ```bash -$ kubectl -n create -f pvc.yaml +$ kubectl -n create -f pvc.yaml ``` #### Configure Jenkins CR From 2a01d66957a35c687a6313811259231edf7b819b Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Fri, 27 Sep 2019 23:14:38 -0400 Subject: [PATCH 04/16] Upgrade to go v1.13 --- .travis.yml | 9 ++++----- config.env | 4 ++-- go.mod | 2 ++ 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5e0fc1f6..fba98145 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,4 @@ language: go -sudo: required env: global: @@ -11,15 +10,15 @@ env: - KUBECONFIG=$HOME/.kube/config go: -- 1.12.x + - 1.13.x matrix: fast_finish: true allow_failures: - - go: master + - go: master before_install: -- make go-dependencies + - make go-dependencies jobs: include: @@ -36,4 +35,4 @@ jobs: cache: directories: - vendor - - /home/travis/.minikube/ \ No newline at end of file + - /home/travis/.minikube/ diff --git a/config.env b/config.env index 129c622c..f07ccefd 100644 --- a/config.env +++ b/config.env @@ -1,7 +1,7 @@ # Setup variables for the Makefile NAME=kubernetes-operator OPERATOR_SDK_VERSION=0.10.0 -GO_VERSION=1.12.6 +GO_VERSION=1.13.1 PKG=github.com/jenkinsci/kubernetes-operator DOCKER_ORGANIZATION=virtuslab DOCKER_REGISTRY=jenkins-operator @@ -12,4 +12,4 @@ MINIKUBE_DRIVER=virtualbox MINIKUBE_VERSION=1.2.0 KUBECTL_CONTEXT=minikube ALL_IN_ONE_DEPLOY_FILE_PREFIX=all-in-one -GEN_CRD_API=gen-crd-api-reference-docs \ No newline at end of file +GEN_CRD_API=gen-crd-api-reference-docs diff --git a/go.mod b/go.mod index 1338153e..0416b5e1 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,7 @@ module github.com/jenkinsci/kubernetes-operator +go 1.13 + require ( github.com/bndr/gojenkins v0.0.0-20181125150310-de43c03cf849 github.com/docker/distribution v2.7.1+incompatible From 648af2009250d5babf39a341b2a0f7d5159db53f Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Sat, 28 Sep 2019 14:22:12 -0400 Subject: [PATCH 05/16] Fix `thrift@v0.12.0 used for two different module paths` --- go.mod | 4 +--- go.sum | 3 +++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 0416b5e1..c2d16cd0 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,7 @@ require ( golang.org/x/net v0.0.0-20190909003024-a7b16738d86b // indirect golang.org/x/sys v0.0.0-20190910064555-bbd175535a8b // indirect golang.org/x/text v0.3.2 // indirect - golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578 // indirect + golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e // indirect k8s.io/api v0.0.0-20190612125737-db0771252981 k8s.io/apimachinery v0.0.0-20190612125636-6a5db36e93ad k8s.io/client-go v11.0.0+incompatible @@ -50,5 +50,3 @@ replace ( sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.1.10 sigs.k8s.io/controller-tools => sigs.k8s.io/controller-tools v0.1.11-0.20190411181648-9d55346c2bde ) - -replace git.apache.org/thrift.git => github.com/apache/thrift v0.12.0 diff --git a/go.sum b/go.sum index 12f607e9..bc3d8a58 100644 --- a/go.sum +++ b/go.sum @@ -10,6 +10,7 @@ contrib.go.opencensus.io/exporter/ocagent v0.4.9 h1:8ZbMXpyd04/3LILa/9Tzr8N4HzZN contrib.go.opencensus.io/exporter/ocagent v0.4.9/go.mod h1:ueLzZcP7LPhPulEBukGn4aLh7Mx9YJwpVJ9nL2FYltw= contrib.go.opencensus.io/exporter/ocagent v0.4.11 h1:Zwy9skaqR2igcEfSVYDuAsbpa33N0RPtnYTHEe2whPI= contrib.go.opencensus.io/exporter/ocagent v0.4.11/go.mod h1:7ihiYRbdcVfW4m4wlXi9WRPdv79C0fStcjNlyE6ek9s= +git.apache.org/thrift.git v0.0.0-20180902110319-2566ecd5d999/go.mod h1:fPE2ZNJGynbRyZ4dJvy6G277gSllfV2HJqblrnkyeyg= git.apache.org/thrift.git v0.12.0/go.mod h1:fPE2ZNJGynbRyZ4dJvy6G277gSllfV2HJqblrnkyeyg= github.com/Azure/go-ansiterm v0.0.0-20170929234023-d6e3b3328b78/go.mod h1:LmzpDX56iTiv29bbRTIsUNlaFfuhWRQBWjQdVyAevI8= github.com/Azure/go-autorest v11.1.0+incompatible/go.mod h1:r+4oMnoxhatjLLJ6zxSWATqVooLgysK6ZNox3g/xq24= @@ -526,6 +527,8 @@ golang.org/x/tools v0.0.0-20190708203411-c8855242db9c h1:rRFNgkkT7zOyWlroLBmsrKY golang.org/x/tools v0.0.0-20190708203411-c8855242db9c/go.mod h1:jcCCGcm9btYwXyDqrUWc6MKQKKGJCWEQ3AfLSRIbEuI= golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578 h1:f0Gfd654rnnfXT1+BK1YHPTS1qQdKrPIaGQwWxNE44k= golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e h1:1xWUkZQQ9Z9UuZgNaIR6OQOE7rUFglXUUBZlO+dGg6I= +golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/api v0.0.0-20180910000450-7ca32eb868bf/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0= google.golang.org/api v0.0.0-20181030000543-1d582fd0359e/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0= From cd7e70df9cb2a5c6dd6b0882e0da8624b798fa06 Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Sat, 28 Sep 2019 14:34:25 -0400 Subject: [PATCH 06/16] Update to use go 1.13-built staticcheck Fix the link --- Makefile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 35e117d6..827418a0 100644 --- a/Makefile +++ b/Makefile @@ -200,10 +200,10 @@ PLATFORM = $(shell echo $(UNAME_S) | tr A-Z a-z) staticcheck: ## Verifies `staticcheck` passes @echo "+ $@" ifndef HAS_STATICCHECK - wget https://github.com/dominikh/go-tools/releases/download/2019.1.1/staticcheck_$(PLATFORM)_amd64 - chmod +x staticcheck_$(PLATFORM)_amd64 + wget -O staticcheck_$(PLATFORM)_amd64.tar.gz https://github.com/dominikh/go-tools/releases/download/2019.2.3/staticcheck_$(PLATFORM)_amd64.tar.gz + tar zxvf staticcheck_$(PLATFORM)_amd64.tar.gz mkdir -p $(GOPATH)/bin - mv staticcheck_$(PLATFORM)_amd64 $(GOPATH)/bin/staticcheck + mv staticcheck/staticcheck $(GOPATH)/bin endif @staticcheck $(PACKAGES) @@ -463,4 +463,4 @@ helm-package: helm-deploy: helm-package @echo "+ $@" helm repo index chart/ --url https://raw.githubusercontent.com/jenkinsci/kubernetes-operator/master/chart/jenkins-operator/ - cd chart/ && mv jenkins-operator-*.tgz jenkins-operator \ No newline at end of file + cd chart/ && mv jenkins-operator-*.tgz jenkins-operator From 04aed2e658c00e6682a3e6642b09afef37f433bc Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Sat, 28 Sep 2019 15:16:27 -0400 Subject: [PATCH 07/16] Upgrade minikube to v1.4.0 --- config.env | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.env b/config.env index f07ccefd..dc9618e6 100644 --- a/config.env +++ b/config.env @@ -9,7 +9,7 @@ NAMESPACE=default API_VERSION=v1alpha2 MINIKUBE_KUBERNETES_VERSION=v1.12.9 MINIKUBE_DRIVER=virtualbox -MINIKUBE_VERSION=1.2.0 +MINIKUBE_VERSION=1.4.0 KUBECTL_CONTEXT=minikube ALL_IN_ONE_DEPLOY_FILE_PREFIX=all-in-one GEN_CRD_API=gen-crd-api-reference-docs From 6ca00d109b38fda0db3136b513a73721f445f414 Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Sat, 28 Sep 2019 15:17:45 -0400 Subject: [PATCH 08/16] Update k8s version to v1.16.0 (default version for minikube v1.4.0) --- config.env | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.env b/config.env index dc9618e6..53037d69 100644 --- a/config.env +++ b/config.env @@ -7,7 +7,7 @@ DOCKER_ORGANIZATION=virtuslab DOCKER_REGISTRY=jenkins-operator NAMESPACE=default API_VERSION=v1alpha2 -MINIKUBE_KUBERNETES_VERSION=v1.12.9 +MINIKUBE_KUBERNETES_VERSION=v1.16.0 MINIKUBE_DRIVER=virtualbox MINIKUBE_VERSION=1.4.0 KUBECTL_CONTEXT=minikube From b7c153f40ca09e51729ed4e5a203143d19bd6051 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Tue, 1 Oct 2019 09:50:38 +0200 Subject: [PATCH 09/16] Improved notification messages --- cmd/manager/main.go | 6 +- pkg/apis/jenkins/v1alpha2/jenkins_types.go | 6 +- .../jenkins/v1alpha2/zz_generated.deepcopy.go | 7 ++ .../jenkins/configuration/base/reconcile.go | 1 + pkg/controller/jenkins/jenkins_controller.go | 108 ++++++++++++++---- .../jenkins/notifications/mailgun.go | 20 ++-- .../jenkins/notifications/msteams.go | 35 ++++-- .../jenkins/notifications/msteams_test.go | 2 +- .../jenkins/notifications/sender.go | 36 +++++- pkg/controller/jenkins/notifications/slack.go | 41 ++++--- .../jenkins/notifications/slack_test.go | 2 +- 11 files changed, 192 insertions(+), 72 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 47f440e6..ac5634e2 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -13,6 +13,7 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/apis" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" + "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" @@ -118,8 +119,11 @@ func main() { fatal(errors.Wrap(err, "failed to create Kubernetes client set"), *debug) } + c := make(chan notifications.Event) + go notifications.Listen(c, mgr.GetClient()) + // setup Jenkins controller - if err := jenkins.Add(mgr, *local, *minikube, events, *clientSet, *cfg); err != nil { + if err := jenkins.Add(mgr, *local, *minikube, events, *clientSet, *cfg, &c); err != nil { fatal(errors.Wrap(err, "failed to setup controllers"), *debug) } diff --git a/pkg/apis/jenkins/v1alpha2/jenkins_types.go b/pkg/apis/jenkins/v1alpha2/jenkins_types.go index cdf98c23..0ae0c136 100644 --- a/pkg/apis/jenkins/v1alpha2/jenkins_types.go +++ b/pkg/apis/jenkins/v1alpha2/jenkins_types.go @@ -17,9 +17,9 @@ type JenkinsSpec struct { // +optional SeedJobs []SeedJob `json:"seedJobs,omitempty"` - /* // Notifications defines list of a services which are used to inform about Jenkins status - // Can be used to integrate chat services like Slack, Microsoft MicrosoftTeams or Mailgun - Notifications []Notification `json:"notifications,omitempty"`*/ + // Notifications defines list of a services which are used to inform about Jenkins status + // Can be used to integrate chat services like Slack, Microsoft Teams or Mailgun + Notifications []Notification `json:"notifications,omitempty"` // Service is Kubernetes service of Jenkins master HTTP pod // Defaults to : diff --git a/pkg/apis/jenkins/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/jenkins/v1alpha2/zz_generated.deepcopy.go index 932750f5..88c845c6 100644 --- a/pkg/apis/jenkins/v1alpha2/zz_generated.deepcopy.go +++ b/pkg/apis/jenkins/v1alpha2/zz_generated.deepcopy.go @@ -342,6 +342,13 @@ func (in *JenkinsSpec) DeepCopyInto(out *JenkinsSpec) { *out = make([]SeedJob, len(*in)) copy(*out, *in) } + if in.Notifications != nil { + in, out := &in.Notifications, &out.Notifications + *out = make([]Notification, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } in.Service.DeepCopyInto(&out.Service) in.SlaveService.DeepCopyInto(&out.SlaveService) in.Backup.DeepCopyInto(&out.Backup) diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 41c0c1ed..4a7cd7bf 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -124,6 +124,7 @@ func (r *ReconcileJenkinsBaseConfiguration) Reconcile() (reconcile.Result, jenki } result, err = r.ensureBaseConfiguration(jenkinsClient) + return result, jenkinsClient, err } diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 0e6361e2..7cf371cb 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,6 +3,7 @@ package jenkins import ( "context" "fmt" + "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" "reflect" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" @@ -52,20 +53,21 @@ var reconcileErrors = map[string]reconcileError{} // Add creates a new Jenkins Controller and adds it to the Manager. The Manager will set fields on the Controller // and Start it when the Manager is Started. -func Add(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config) error { - return add(mgr, newReconciler(mgr, local, minikube, events, clientSet, config)) +func Add(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) error { + return add(mgr, newReconciler(mgr, local, minikube, events, clientSet, config, notificationEvents)) } // newReconciler returns a new reconcile.Reconciler -func newReconciler(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config) reconcile.Reconciler { +func newReconciler(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) reconcile.Reconciler { return &ReconcileJenkins{ - client: mgr.GetClient(), - scheme: mgr.GetScheme(), - local: local, - minikube: minikube, - events: events, - clientSet: clientSet, - config: config, + client: mgr.GetClient(), + scheme: mgr.GetScheme(), + local: local, + minikube: minikube, + events: events, + clientSet: clientSet, + config: config, + notificationEvents: notificationEvents, } } @@ -119,19 +121,34 @@ var _ reconcile.Reconciler = &ReconcileJenkins{} // ReconcileJenkins reconciles a Jenkins object type ReconcileJenkins struct { - client client.Client - scheme *runtime.Scheme - local, minikube bool - events event.Recorder - clientSet kubernetes.Clientset - config rest.Config + client client.Client + scheme *runtime.Scheme + local, minikube bool + events event.Recorder + clientSet kubernetes.Clientset + config rest.Config + notificationEvents *chan notifications.Event } // Reconcile it's a main reconciliation loop which maintain desired state based on Jenkins.Spec func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Result, error) { + reconcileFailLimit := uint64(10) logger := r.buildLogger(request.Name) logger.V(log.VDebug).Info("Reconciling Jenkins") + jenkins := &v1alpha2.Jenkins{} + err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. + // Return and don't requeue + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, errors.WithStack(err) + } + result, err := r.reconcile(request, logger) if err != nil && apierrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil @@ -151,11 +168,19 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul } } reconcileErrors[request.Name] = lastErrors - if lastErrors.counter >= 15 { + if lastErrors.counter >= reconcileFailLimit { if log.Debug { - logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err)) + logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %+v", reconcileFailLimit, err)) } else { - logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %s", err)) + logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %s", reconcileFailLimit, err)) + } + + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + ConfigurationType: notifications.ConfigurationTypeUnknown, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %s", err), + MessageVerbose: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err), } return reconcile.Result{Requeue: false}, nil } @@ -203,8 +228,16 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg return reconcile.Result{}, err } if !valid { + message := "Validation of base configuration failed, please correct Jenkins CR" + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + ConfigurationType: notifications.ConfigurationTypeBase, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: message, + MessageVerbose: message, + } r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "Base CR validation failed") - logger.V(log.VWarn).Info("Validation of base configuration failed, please correct Jenkins CR") + logger.V(log.VWarn).Info(message) return reconcile.Result{}, nil // don't requeue } @@ -226,8 +259,17 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg if err != nil { return reconcile.Result{}, errors.WithStack(err) } - logger.Info(fmt.Sprintf("Base configuration phase is complete, took %s", - jenkins.Status.BaseConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time))) + + message := fmt.Sprintf("Base configuration phase is complete, took %s", + jenkins.Status.BaseConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time)) + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + ConfigurationType: notifications.ConfigurationTypeBase, + LogLevel: v1alpha2.NotificationLogLevelInfo, + Message: message, + MessageVerbose: message, + } + logger.Info(message) r.events.Emit(jenkins, event.TypeNormal, reasonBaseConfigurationSuccess, "Base configuration completed") } // Reconcile user configuration @@ -238,7 +280,15 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg return reconcile.Result{}, err } if !valid { - logger.V(log.VWarn).Info("Validation of user configuration failed, please correct Jenkins CR") + message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + ConfigurationType: notifications.ConfigurationTypeUser, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: message, + MessageVerbose: message, + } + logger.V(log.VWarn).Info(message) r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "User CR validation failed") return reconcile.Result{}, nil // don't requeue } @@ -258,8 +308,16 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg if err != nil { return reconcile.Result{}, errors.WithStack(err) } - logger.Info(fmt.Sprintf("User configuration phase is complete, took %s", - jenkins.Status.UserConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time))) + message := fmt.Sprintf("User configuration phase is complete, took %s", + jenkins.Status.UserConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time)) + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + ConfigurationType: notifications.ConfigurationTypeUser, + LogLevel: v1alpha2.NotificationLogLevelInfo, + Message: message, + MessageVerbose: message, + } + logger.Info(message) r.events.Emit(jenkins, event.TypeNormal, reasonUserConfigurationSuccess, "User configuration completed") } diff --git a/pkg/controller/jenkins/notifications/mailgun.go b/pkg/controller/jenkins/notifications/mailgun.go index 4aec5ff6..8e705c56 100644 --- a/pkg/controller/jenkins/notifications/mailgun.go +++ b/pkg/controller/jenkins/notifications/mailgun.go @@ -20,8 +20,8 @@ const content = ` -

Jenkins Operator Reconciled

-

Failed to do something

+

%s

+

%s

@@ -31,10 +31,6 @@ const content = ` - - - -
CR name:Configuration type: %s
Status:%s
Powered by Jenkins Operator <3
@@ -74,9 +70,17 @@ func (m MailGun) Send(event Event, config v1alpha2.Notification) error { mg := mailgun.NewMailgun(config.Mailgun.Domain, secretValue) - htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), event.Jenkins.Name, event.ConfigurationType, m.getStatusColor(event.LogLevel), string(event.LogLevel)) + var statusMessage string - msg := mg.NewMessage(fmt.Sprintf("Jenkins Operator Notifier <%s>", config.Mailgun.From), "Jenkins Operator Status", "", config.Mailgun.Recipient) + if config.Verbose { + statusMessage = event.MessageVerbose + } else { + statusMessage = event.Message + } + + htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), statusMessage, event.Jenkins.Name, event.ConfigurationType, m.getStatusColor(event.LogLevel)) + + msg := mg.NewMessage(fmt.Sprintf("Jenkins Operator Notifier <%s>", config.Mailgun.From), notificationTitle(event), "", config.Mailgun.Recipient) msg.SetHtml(htmlMessage) ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) defer cancel() diff --git a/pkg/controller/jenkins/notifications/msteams.go b/pkg/controller/jenkins/notifications/msteams.go index 8cbfa679..76a6747a 100644 --- a/pkg/controller/jenkins/notifications/msteams.go +++ b/pkg/controller/jenkins/notifications/msteams.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "net/http" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" @@ -26,6 +27,7 @@ type TeamsMessage struct { ThemeColor StatusColor `json:"themeColor"` Title string `json:"title"` Sections []TeamsSection `json:"sections"` + Summary string `json:"summary"` } // TeamsSection is MS Teams message section @@ -67,11 +69,10 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { return errors.Errorf("Microsoft Teams webhook URL is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) } - msg, err := json.Marshal(TeamsMessage{ + tm := &TeamsMessage{ Type: "MessageCard", Context: "https://schema.org/extensions", ThemeColor: t.getStatusColor(event.LogLevel), - Title: titleText, Sections: []TeamsSection{ { Facts: []TeamsFact{ @@ -79,14 +80,6 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { Name: crNameFieldName, Value: event.Jenkins.Name, }, - { - Name: configurationTypeFieldName, - Value: event.ConfigurationType, - }, - { - Name: loggingLevelFieldName, - Value: string(event.LogLevel), - }, { Name: namespaceFieldName, Value: event.Jenkins.Namespace, @@ -95,7 +88,24 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { Text: event.Message, }, }, - }) + Summary: event.Message, + } + + tm.Title = notificationTitle(event) + + if config.Verbose { + tm.Sections[0].Text = event.MessageVerbose + tm.Summary = event.MessageVerbose + } + + if event.ConfigurationType != ConfigurationTypeUnknown { + tm.Sections[0].Facts = append(tm.Sections[0].Facts, TeamsFact{ + Name: configurationTypeFieldName, + Value: event.ConfigurationType, + }) + } + + msg, err := json.Marshal(tm) if err != nil { return errors.WithStack(err) } @@ -110,6 +120,9 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { return errors.WithStack(err) } + if resp.StatusCode != http.StatusOK { + return errors.New(fmt.Sprintf("Invalid response from server: %s", resp.Status)) + } defer func() { _ = resp.Body.Close() }() return nil diff --git a/pkg/controller/jenkins/notifications/msteams_test.go b/pkg/controller/jenkins/notifications/msteams_test.go index c62afa2b..36ea3cde 100644 --- a/pkg/controller/jenkins/notifications/msteams_test.go +++ b/pkg/controller/jenkins/notifications/msteams_test.go @@ -42,7 +42,7 @@ func TestTeams_Send(t *testing.T) { t.Fatal(err) } - assert.Equal(t, message.Title, titleText) + assert.Equal(t, message.Title, notificationTitle(event)) assert.Equal(t, message.ThemeColor, teams.getStatusColor(event.LogLevel)) mainSection := message.Sections[0] diff --git a/pkg/controller/jenkins/notifications/sender.go b/pkg/controller/jenkins/notifications/sender.go index fd20f82c..3b12e5bc 100644 --- a/pkg/controller/jenkins/notifications/sender.go +++ b/pkg/controller/jenkins/notifications/sender.go @@ -1,13 +1,19 @@ package notifications import ( + "fmt" "net/http" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" + "github.com/jenkinsci/kubernetes-operator/pkg/log" + + "github.com/pkg/errors" + k8sclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( - titleText = "Operator reconciled." + infoTitleText = "Jenkins Operator reconciliation info" + warnTitleText = "Jenkins Operator reconciliation warning" messageFieldName = "Message" loggingLevelFieldName = "Logging Level" crNameFieldName = "CR Name" @@ -16,6 +22,17 @@ const ( footerContent = "Powered by Jenkins Operator" ) +const ( + // ConfigurationTypeBase is core configuration of Jenkins provided by the Operator + ConfigurationTypeBase = "base" + + // ConfigurationTypeUser is user-defined configuration of Jenkins + ConfigurationTypeUser = "user" + + // ConfigurationTypeUnknown is untraceable type of configuration + ConfigurationTypeUnknown = "unknown" +) + var ( testConfigurationType = "test-configuration" testCrName = "test-cr" @@ -42,7 +59,7 @@ type Event struct { MessageVerbose string } -/*type service interface { +type service interface { Send(event Event, notificationConfig v1alpha2.Notification) error } @@ -61,7 +78,7 @@ func Listen(events chan Event, k8sClient k8sclient.Client) { } else if notificationConfig.Mailgun != nil { svc = MailGun{k8sClient: k8sClient} } else { - logger.V(log.VWarn).Info(fmt.Sprintf("Unexpected notification `%+v`", notificationConfig)) + logger.V(log.VWarn).Info(fmt.Sprintf("Unknown notification service `%+v`", notificationConfig)) continue } @@ -85,6 +102,15 @@ func notify(svc service, event Event, manifest v1alpha2.Notification) error { if event.LogLevel == v1alpha2.NotificationLogLevelInfo && manifest.LoggingLevel == v1alpha2.NotificationLogLevelWarning { return nil } - return svc.Send(event, manifest) -}*/ +} + +func notificationTitle(event Event) string { + if event.LogLevel == v1alpha2.NotificationLogLevelInfo { + return infoTitleText + } else if event.LogLevel == v1alpha2.NotificationLogLevelWarning { + return warnTitleText + } else { + return "undefined" + } +} diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index 8b817af8..a48cfd49 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -64,12 +64,11 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { return err } - slackMessage, err := json.Marshal(SlackMessage{ + sm := &SlackMessage{ Attachments: []SlackAttachment{ { Fallback: "", Color: s.getStatusColor(event.LogLevel), - Text: titleText, Fields: []SlackField{ { Title: messageFieldName, @@ -81,16 +80,6 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { Value: event.Jenkins.Name, Short: true, }, - { - Title: configurationTypeFieldName, - Value: event.ConfigurationType, - Short: true, - }, - { - Title: loggingLevelFieldName, - Value: string(event.LogLevel), - Short: true, - }, { Title: namespaceFieldName, Value: event.Jenkins.Namespace, @@ -100,17 +89,35 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { Footer: footerContent, }, }, - }) + } + + mainAttachment := sm.Attachments[0] + + mainAttachment.Title = notificationTitle(event) + + if config.Verbose { + // TODO: or for title == message + mainAttachment.Fields[0].Value = event.MessageVerbose + } + + if event.ConfigurationType != ConfigurationTypeUnknown { + mainAttachment.Fields = append(mainAttachment.Fields, SlackField{ + Title: configurationTypeFieldName, + Value: event.ConfigurationType, + Short: true, + }) + } + + slackMessage, err := json.Marshal(sm) + if err != nil { + return err + } secretValue := string(secret.Data[selector.Key]) if secretValue == "" { return errors.Errorf("SecretValue %s is empty", selector.Name) } - if err != nil { - return err - } - request, err := http.NewRequest("POST", secretValue, bytes.NewBuffer(slackMessage)) if err != nil { return err diff --git a/pkg/controller/jenkins/notifications/slack_test.go b/pkg/controller/jenkins/notifications/slack_test.go index e4441ba0..d40b7c71 100644 --- a/pkg/controller/jenkins/notifications/slack_test.go +++ b/pkg/controller/jenkins/notifications/slack_test.go @@ -45,7 +45,7 @@ func TestSlack_Send(t *testing.T) { mainAttachment := message.Attachments[0] - assert.Equal(t, mainAttachment.Text, titleText) + assert.Equal(t, mainAttachment.Title, notificationTitle(event)) for _, field := range mainAttachment.Fields { switch field.Title { case configurationTypeFieldName: From b67996880a1d149e69b15b3c7271558cd3310636 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Tue, 1 Oct 2019 10:14:13 +0200 Subject: [PATCH 10/16] Fix slack notifications --- pkg/controller/jenkins/notifications/slack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index a48cfd49..d1081fcb 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -91,7 +91,7 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { }, } - mainAttachment := sm.Attachments[0] + mainAttachment := &sm.Attachments[0] mainAttachment.Title = notificationTitle(event) From ca3508ef499b604c173c42a50af4568d87375956 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Thu, 3 Oct 2019 10:19:31 +0200 Subject: [PATCH 11/16] Add kubernetes event as notification provider, imrpove notification warnings --- cmd/manager/main.go | 4 +- .../backuprestore/backuprestore.go | 27 +- .../jenkins/configuration/base/validate.go | 244 ++++++++---------- .../configuration/base/validate_test.go | 80 +++--- .../configuration/user/seedjobs/validate.go | 119 ++++----- .../user/seedjobs/validate_test.go | 40 +-- .../jenkins/configuration/user/validate.go | 6 +- pkg/controller/jenkins/jenkins_controller.go | 79 +++--- .../jenkins/notifications/mailgun.go | 9 +- .../jenkins/notifications/msteams.go | 10 +- .../jenkins/notifications/msteams_test.go | 4 +- .../jenkins/notifications/sender.go | 38 ++- pkg/controller/jenkins/notifications/slack.go | 10 +- .../jenkins/notifications/slack_test.go | 4 +- 14 files changed, 326 insertions(+), 348 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ac5634e2..ccab3762 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -120,10 +120,10 @@ func main() { } c := make(chan notifications.Event) - go notifications.Listen(c, mgr.GetClient()) + go notifications.Listen(c, events, mgr.GetClient()) // setup Jenkins controller - if err := jenkins.Add(mgr, *local, *minikube, events, *clientSet, *cfg, &c); err != nil { + if err := jenkins.Add(mgr, *local, *minikube, *clientSet, *cfg, &c); err != nil { fatal(errors.Wrap(err, "failed to setup controllers"), *debug) } diff --git a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go index d9f4701f..85b85d49 100644 --- a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go +++ b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go @@ -46,8 +46,8 @@ func New(k8sClient k8s.Client, clientSet kubernetes.Clientset, } // Validate validates backup and restore configuration -func (bar *BackupAndRestore) Validate() bool { - valid := true +func (bar *BackupAndRestore) Validate() []string { + var messages []string allContainers := map[string]v1alpha2.Container{} for _, container := range bar.jenkins.Spec.Master.Containers { allContainers[container.Name] = container @@ -57,12 +57,10 @@ func (bar *BackupAndRestore) Validate() bool { if len(restore.ContainerName) > 0 { _, found := allContainers[restore.ContainerName] if !found { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("restore container '%s' not found in CR spec.master.containers", restore.ContainerName)) + messages = append(messages, fmt.Sprintf("restore container '%s' not found in CR spec.master.containers", restore.ContainerName)) } if restore.Action.Exec == nil { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.restore.action.exec is not configured")) + messages = append(messages, fmt.Sprintf("spec.restore.action.exec is not configured")) } } @@ -70,29 +68,24 @@ func (bar *BackupAndRestore) Validate() bool { if len(backup.ContainerName) > 0 { _, found := allContainers[backup.ContainerName] if !found { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("backup container '%s' not found in CR spec.master.containers", backup.ContainerName)) + messages = append(messages, fmt.Sprintf("backup container '%s' not found in CR spec.master.containers", backup.ContainerName)) } if backup.Action.Exec == nil { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.backup.action.exec is not configured")) + messages = append(messages, fmt.Sprintf("spec.backup.action.exec is not configured")) } if backup.Interval == 0 { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.backup.interval is not configured")) + messages = append(messages, fmt.Sprintf("spec.backup.interval is not configured")) } } if len(restore.ContainerName) > 0 && len(backup.ContainerName) == 0 { - valid = false - bar.logger.V(log.VWarn).Info("spec.backup.containerName is not configured") + messages = append(messages, "spec.backup.containerName is not configured") } if len(backup.ContainerName) > 0 && len(restore.ContainerName) == 0 { - valid = false - bar.logger.V(log.VWarn).Info("spec.restore.containerName is not configured") + messages = append(messages, "spec.restore.containerName is not configured") } - return valid + return messages } // Restore performs Jenkins restore backup operation diff --git a/pkg/controller/jenkins/configuration/base/validate.go b/pkg/controller/jenkins/configuration/base/validate.go index 31a2a803..47c4df7b 100644 --- a/pkg/controller/jenkins/configuration/base/validate.go +++ b/pkg/controller/jenkins/configuration/base/validate.go @@ -24,213 +24,207 @@ var ( ) // Validate validates Jenkins CR Spec.master section -func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) (bool, error) { - if !r.validateReservedVolumes() { - return false, nil +func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) ([]string, error) { + var messages []string + + if msg := r.validateReservedVolumes(); msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateVolumes(); err != nil { - return false, err - } else if !valid { - return false, nil + + if msg, err := r.validateVolumes(); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } for _, container := range jenkins.Spec.Master.Containers { - if !r.validateContainer(container) { - return false, nil + if msg := r.validateContainer(container); msg != nil { + messages = append(messages, msg...) } } - if !r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins) { - return false, nil + if msg := r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins); msg != nil { + messages = append(messages, msg...) } - if !r.validateJenkinsMasterPodEnvs() { - return false, nil + if msg := r.validateJenkinsMasterPodEnvs(); msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateCustomization(r.jenkins.Spec.GroovyScripts.Customization, "spec.groovyScripts"); err != nil { - return false, err - } else if !valid { - return false, nil + if msg, err := r.validateCustomization(r.jenkins.Spec.GroovyScripts.Customization, "spec.groovyScripts"); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateCustomization(r.jenkins.Spec.ConfigurationAsCode.Customization, "spec.configurationAsCode"); err != nil { - return false, err - } else if !valid { - return false, nil + if msg, err := r.validateCustomization(r.jenkins.Spec.ConfigurationAsCode.Customization, "spec.configurationAsCode"); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecrets() (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecrets() ([]string, error) { + var messages []string for _, sr := range r.jenkins.Spec.Master.ImagePullSecrets { - valid, err := r.validateImagePullSecret(sr.Name) + msg, err := r.validateImagePullSecret(sr.Name) if err != nil { - return false, err + return nil, err } - if !valid { - return false, nil + if msg != nil { + messages = append(messages, msg...) } } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecret(secretName string) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecret(secretName string) ([]string, error) { + var messages []string secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret %s not found defined in spec.master.imagePullSecrets", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret %s not found defined in spec.master.imagePullSecrets", secretName)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } if secret.Data["docker-server"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key.", secretName)) } if secret.Data["docker-username"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key.", secretName)) } if secret.Data["docker-password"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key.", secretName)) } if secret.Data["docker-email"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key.", secretName)) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateVolumes() (bool, error) { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateVolumes() ([]string, error) { + var messages []string for _, volume := range r.jenkins.Spec.Master.Volumes { switch { case volume.ConfigMap != nil: - if ok, err := r.validateConfigMapVolume(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validateConfigMapVolume(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } case volume.Secret != nil: - if ok, err := r.validateSecretVolume(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validateSecretVolume(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } case volume.PersistentVolumeClaim != nil: - if ok, err := r.validatePersistentVolumeClaim(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validatePersistentVolumeClaim(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } default: //TODO add support for rest of volumes - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Unsupported volume '%v'", volume)) + messages = append(messages, fmt.Sprintf("Unsupported volume '%v'", volume)) } } - return valid, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validatePersistentVolumeClaim(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validatePersistentVolumeClaim(volume corev1.Volume) ([]string, error) { + var messages []string + pvc := &corev1.PersistentVolumeClaim{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.PersistentVolumeClaim.ClaimName, Namespace: r.jenkins.ObjectMeta.Namespace}, pvc) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("PersistentVolumeClaim '%s' not found for volume '%v'", volume.PersistentVolumeClaim.ClaimName, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("PersistentVolumeClaim '%s' not found for volume '%v'", volume.PersistentVolumeClaim.ClaimName, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateConfigMapVolume(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateConfigMapVolume(volume corev1.Volume) ([]string, error) { + var messages []string if volume.ConfigMap.Optional != nil && *volume.ConfigMap.Optional { - return true, nil + return nil, nil } configMap := &corev1.ConfigMap{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.ConfigMap.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, configMap) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("ConfigMap '%s' not found for volume '%v'", volume.ConfigMap.Name, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("ConfigMap '%s' not found for volume '%v'", volume.ConfigMap.Name, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateSecretVolume(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateSecretVolume(volume corev1.Volume) ([]string, error) { + var messages []string if volume.Secret.Optional != nil && *volume.Secret.Optional { - return true, nil + return nil, nil } secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.Secret.SecretName, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' not found for volume '%v'", volume.Secret.SecretName, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' not found for volume '%v'", volume.Secret.SecretName, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateReservedVolumes() bool { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateReservedVolumes() []string { + var messages []string for _, baseVolume := range resources.GetJenkinsMasterPodBaseVolumes(r.jenkins) { for _, volume := range r.jenkins.Spec.Master.Volumes { if baseVolume.Name == volume.Name { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master pod volume '%s' is reserved please choose different one", volume.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Jenkins Master pod volume '%s' is reserved please choose different one", volume.Name)) } } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateContainer(container v1alpha2.Container) bool { - logger := r.logger.WithValues("container", container.Name) +func (r *ReconcileJenkinsBaseConfiguration) validateContainer(container v1alpha2.Container) []string { + var messages []string if container.Image == "" { - logger.V(log.VWarn).Info("Image not set") - return false + messages = append(messages, "Image not set") } if !dockerImageRegexp.MatchString(container.Image) && !docker.ReferenceRegexp.MatchString(container.Image) { - logger.V(log.VWarn).Info("Invalid image") - return false + messages = append(messages, "Invalid image") } if container.ImagePullPolicy == "" { - logger.V(log.VWarn).Info("Image pull policy not set") - return false + messages = append(messages, "Image pull policy not set") } - if !r.validateContainerVolumeMounts(container) { - return false + if msg := r.validateContainerVolumeMounts(container); msg != nil { + messages = append(messages, msg...) } - return true + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(container v1alpha2.Container) bool { - logger := r.logger.WithValues("container", container.Name) +func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(container v1alpha2.Container) []string { + var messages []string allVolumes := append(resources.GetJenkinsMasterPodBaseVolumes(r.jenkins), r.jenkins.Spec.Master.Volumes...) - valid := true for _, volumeMount := range container.VolumeMounts { if len(volumeMount.MountPath) == 0 { - logger.V(log.VWarn).Info(fmt.Sprintf("mountPath not set for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) - valid = false + messages = append(messages, fmt.Sprintf("mountPath not set for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) } foundVolume := false @@ -241,15 +235,15 @@ func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(contai } if !foundVolume { - logger.V(log.VWarn).Info(fmt.Sprintf("Not found volume for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Not found volume for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool { +func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() []string { + var messages []string baseEnvs := resources.GetJenkinsMasterContainerBaseEnvs(r.jenkins) baseEnvNames := map[string]string{} for _, env := range baseEnvs { @@ -257,14 +251,12 @@ func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool } javaOpts := corev1.EnvVar{} - valid := true for _, userEnv := range r.jenkins.Spec.Master.Containers[0].Env { if userEnv.Name == constants.JavaOpsVariableName { javaOpts = userEnv } if _, overriding := baseEnvNames[userEnv.Name]; overriding { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master container env '%s' cannot be overridden", userEnv.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Jenkins Master container env '%s' cannot be overridden", userEnv.Name)) } } @@ -282,23 +274,21 @@ func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool } for requiredFlag, set := range requiredFlags { if !set { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master container env '%s' doesn't have required flag '%s'", constants.JavaOpsVariableName, requiredFlag)) + messages = append(messages, fmt.Sprintf("Jenkins Master container env '%s' doesn't have required flag '%s'", constants.JavaOpsVariableName, requiredFlag)) } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins []plugins.Plugin, basePlugins, userPlugins []v1alpha2.Plugin) bool { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins []plugins.Plugin, basePlugins, userPlugins []v1alpha2.Plugin) []string { + var messages []string allPlugins := map[plugins.Plugin][]plugins.Plugin{} for _, jenkinsPlugin := range basePlugins { plugin, err := plugins.NewPlugin(jenkinsPlugin.Name, jenkinsPlugin.Version) if err != nil { - r.logger.V(log.VWarn).Info(err.Error()) - valid = false + messages = append(messages, err.Error()) } if plugin != nil { @@ -309,8 +299,7 @@ func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins for _, jenkinsPlugin := range userPlugins { plugin, err := plugins.NewPlugin(jenkinsPlugin.Name, jenkinsPlugin.Version) if err != nil { - r.logger.V(log.VWarn).Info(err.Error()) - valid = false + messages = append(messages, err.Error()) } if plugin != nil { @@ -319,14 +308,14 @@ func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins } if !plugins.VerifyDependencies(allPlugins) { - valid = false + messages = append(messages, "Invalid dependencies") } if !r.verifyBasePlugins(requiredBasePlugins, basePlugins) { - valid = false + messages = append(messages, "Failed to verify base plugins") } - return valid + return messages } func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugins []plugins.Plugin, basePlugins []v1alpha2.Plugin) bool { @@ -349,44 +338,39 @@ func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugin return valid } -func (r *ReconcileJenkinsBaseConfiguration) validateCustomization(customization v1alpha2.Customization, name string) (bool, error) { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateCustomization(customization v1alpha2.Customization, name string) ([]string, error) { + var messages []string if len(customization.Secret.Name) == 0 && len(customization.Configurations) == 0 { - return true, nil + return nil, nil } if len(customization.Secret.Name) > 0 && len(customization.Configurations) == 0 { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("%s.secret.name is set but %s.configurations is empty", name, name)) + messages = append(messages, fmt.Sprintf("%s.secret.name is set but %s.configurations is empty", name, name)) } if len(customization.Secret.Name) > 0 { secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: customization.Secret.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' configured in %s.secret.name not found", customization.Secret.Name, name)) + messages = append(messages, fmt.Sprintf("Secret '%s' configured in %s.secret.name not found", customization.Secret.Name, name)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } } for index, configMapRef := range customization.Configurations { if len(configMapRef.Name) == 0 { - r.logger.V(log.VWarn).Info(fmt.Sprintf("%s.configurations[%d] name is empty", name, index)) - valid = false + messages = append(messages, fmt.Sprintf("%s.configurations[%d] name is empty", name, index)) continue } configMap := &corev1.ConfigMap{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: configMapRef.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, configMap) if err != nil && apierrors.IsNotFound(err) { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("ConfigMap '%s' configured in %s.configurations[%d] not found", configMapRef.Name, name, index)) - return false, nil + messages = append(messages, fmt.Sprintf("ConfigMap '%s' configured in %s.configurations[%d] not found", configMapRef.Name, name, index)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } } - return valid, nil + return messages, nil } diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index 145534f8..ca231a73 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -30,7 +30,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("valid user plugin", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -39,7 +39,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid user plugin name", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -48,7 +48,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("invalid user plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -57,7 +57,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("valid base plugin", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -66,7 +66,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid base plugin name", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -75,7 +75,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("invalid base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -84,7 +84,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("valid user and base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -93,7 +93,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid user and base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -102,7 +102,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("required base plugin set with the same version", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} @@ -111,7 +111,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("required base plugin set with different version", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} @@ -120,16 +120,16 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) - t.Run("missign required base plugin", func(t *testing.T) { + t.Run("missing required base plugin", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} var basePlugins []v1alpha2.Plugin var userPlugins []v1alpha2.Plugin got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -165,7 +165,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, err := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, true) + assert.Nil(t, got) assert.NoError(t, err) }) @@ -186,7 +186,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker email", func(t *testing.T) { @@ -219,7 +219,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker password", func(t *testing.T) { @@ -252,7 +252,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker username", func(t *testing.T) { @@ -285,7 +285,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker server", func(t *testing.T) { @@ -318,7 +318,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) } @@ -348,7 +348,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("override JENKINS_HOME env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -374,7 +374,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing -Djava.awt.headless=true in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -396,7 +396,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing -Djenkins.install.runSetupWizard=false in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -418,7 +418,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -438,7 +438,7 @@ func TestValidateReservedVolumes(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateReservedVolumes() - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("used reserved name", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -455,7 +455,7 @@ func TestValidateReservedVolumes(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateReservedVolumes() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -469,7 +469,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(v1alpha2.Container{}) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("one extra volume", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -496,7 +496,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("empty mountPath", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -523,7 +523,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing volume", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -545,7 +545,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -568,7 +568,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("happy, required", func(t *testing.T) { optional := false @@ -594,7 +594,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("missing configmap", func(t *testing.T) { optional := false @@ -618,7 +618,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -641,7 +641,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("happy, required", func(t *testing.T) { optional := false @@ -665,7 +665,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("missing secret", func(t *testing.T) { optional := false @@ -687,7 +687,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -709,7 +709,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("secret set but configurations is empty", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -731,7 +731,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("secret and configmap exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -761,7 +761,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("secret not exists and configmap exists", func(t *testing.T) { configMapName := "configmap-name" @@ -784,7 +784,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("secret exists and configmap not exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -806,6 +806,6 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go index 75fa4b68..d97dfd56 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go @@ -19,51 +19,44 @@ import ( ) // ValidateSeedJobs verify seed jobs configuration -func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) (bool, error) { - valid := true +func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) ([]string, error) { + var messages []string - if !s.validateIfIDIsUnique(jenkins.Spec.SeedJobs) { - valid = false + if msg := s.validateIfIDIsUnique(jenkins.Spec.SeedJobs); msg != nil { + messages = append(messages, msg...) } for _, seedJob := range jenkins.Spec.SeedJobs { logger := s.logger.WithValues("seedJob", seedJob.ID).V(log.VWarn) if len(seedJob.ID) == 0 { - logger.Info("id can't be empty") - valid = false + messages = append(messages, "id can't be empty") } if len(seedJob.RepositoryBranch) == 0 { - logger.Info("repository branch can't be empty") - valid = false + messages = append(messages, "repository branch can't be empty") } if len(seedJob.RepositoryURL) == 0 { - logger.Info("repository URL branch can't be empty") - valid = false + messages = append(messages, "repository URL branch can't be empty") } if len(seedJob.Targets) == 0 { - logger.Info("targets can't be empty") - valid = false + messages = append(messages, "targets can't be empty") } if _, ok := v1alpha2.AllowedJenkinsCredentialMap[string(seedJob.JenkinsCredentialType)]; !ok { - logger.Info("unknown credential type") - return false, nil + messages = append(messages, "unknown credential type") } if (seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType) && len(seedJob.CredentialID) == 0 { - logger.Info("credential ID can't be empty") - valid = false + messages = append(messages, "credential ID can't be empty") } // validate repository url match private key if strings.Contains(seedJob.RepositoryURL, "git@") && seedJob.JenkinsCredentialType == v1alpha2.NoJenkinsCredentialCredentialType { - logger.Info("Jenkins credential must be set while using ssh repository url") - valid = false + messages = append(messages, "Jenkins credential must be set while using ssh repository url") } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { @@ -71,56 +64,56 @@ func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) (bool, error) { namespaceName := types.NamespacedName{Namespace: jenkins.Namespace, Name: seedJob.CredentialID} err := s.k8sClient.Get(context.TODO(), namespaceName, secret) if err != nil && apierrors.IsNotFound(err) { - logger.Info(fmt.Sprintf("required secret '%s' with Jenkins credential not found", seedJob.CredentialID)) - return false, nil + messages = append(messages, fmt.Sprintf("required secret '%s' with Jenkins credential not found", seedJob.CredentialID)) } else if err != nil { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType { - if ok := validateBasicSSHSecret(logger, *secret); !ok { - valid = false + if msg := validateBasicSSHSecret(logger, *secret); msg != nil { + messages = append(messages, msg...) } } if seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { - if ok := validateUsernamePasswordSecret(logger, *secret); !ok { - valid = false + if msg := validateUsernamePasswordSecret(logger, *secret); msg != nil { + messages = append(messages, msg...) } } } if len(seedJob.BuildPeriodically) > 0 { - if !s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically") { - valid = false + if msg := s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically"); msg != nil { + messages = append(messages, msg...) } } if len(seedJob.PollSCM) > 0 { - if !s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM") { - valid = false + if msg := s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM"); msg != nil { + messages = append(messages, msg...) } } if seedJob.GitHubPushTrigger { - if !s.validateGitHubPushTrigger(jenkins) { - valid = false + if msg := s.validateGitHubPushTrigger(jenkins); msg != nil { + messages = append(messages, msg...) } } } - return valid, nil + return messages, nil } -func (s *SeedJobs) validateSchedule(job v1alpha2.SeedJob, str string, key string) bool { +func (s *SeedJobs) validateSchedule(job v1alpha2.SeedJob, str string, key string) []string { + var messages []string _, err := cron.Parse(str) if err != nil { - s.logger.V(log.VWarn).Info(fmt.Sprintf("`%s` schedule '%s' is invalid cron spec in `%s`", key, str, job.ID)) - return false + messages = append(messages, fmt.Sprintf("`%s` schedule '%s' is invalid cron spec in `%s`", key, str, job.ID)) } - return true + return messages } -func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) bool { +func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) []string { + var messages []string exists := false for _, plugin := range jenkins.Spec.Master.BasePlugins { if plugin.Name == "github" { @@ -136,75 +129,65 @@ func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) bool { } if !exists && !userExists { - s.logger.V(log.VWarn).Info("githubPushTrigger is set. This function requires `github` plugin installed in .Spec.Master.Plugins because seed jobs Push Trigger function needs it") - return false + messages = append(messages, "githubPushTrigger is set. This function requires `github` plugin installed in .Spec.Master.Plugins because seed jobs Push Trigger function needs it") } - return true + return messages } -func (s *SeedJobs) validateIfIDIsUnique(seedJobs []v1alpha2.SeedJob) bool { +func (s *SeedJobs) validateIfIDIsUnique(seedJobs []v1alpha2.SeedJob) []string { + var messages []string ids := map[string]bool{} for _, seedJob := range seedJobs { if _, found := ids[seedJob.ID]; found { - s.logger.V(log.VWarn).Info(fmt.Sprintf("'%s' seed job ID is not unique", seedJob.ID)) - return false + messages = append(messages, fmt.Sprintf("'%s' seed job ID is not unique", seedJob.ID)) } ids[seedJob.ID] = true } - return true + return messages } -func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) bool { - valid := true +func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) []string { + var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } if len(username) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } privateKey, exists := secret.Data[PrivateKeySecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) } if len(string(privateKey)) == 0 { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) - return false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) } if err := validatePrivateKey(string(privateKey)); err != nil { - logger.Info(fmt.Sprintf("private key '%s' invalid in secret '%s': %s", PrivateKeySecretKey, secret.ObjectMeta.Name, err)) - valid = false + messages = append(messages, fmt.Sprintf("private key '%s' invalid in secret '%s': %s", PrivateKeySecretKey, secret.ObjectMeta.Name, err)) } - return valid + return messages } -func validateUsernamePasswordSecret(logger logr.InfoLogger, secret v1.Secret) bool { - valid := true +func validateUsernamePasswordSecret(logger logr.InfoLogger, secret v1.Secret) []string { + var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } if len(username) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } password, exists := secret.Data[PasswordSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) } if len(password) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) } - return valid + return messages } func validatePrivateKey(privateKey string) error { diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go index 864cafe9..707db9a8 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go @@ -76,7 +76,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid without id", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -96,7 +96,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Valid with private key and secret", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -129,7 +129,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid private key in secret", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -162,7 +162,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with PrivateKey and empty Secret data", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -195,7 +195,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with ssh RepositoryURL and empty PrivateKey", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -217,7 +217,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without targets", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -237,7 +237,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without repository URL", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -257,7 +257,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without repository branch", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -277,7 +277,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Valid with username and password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -310,7 +310,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid with empty username", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -343,7 +343,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with empty password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -376,7 +376,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without username", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -408,7 +408,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -440,7 +440,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with wrong cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -463,7 +463,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.False(t, result) + assert.NotNil(t, result) }) t.Run("Valid with good cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -487,7 +487,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.True(t, result) + assert.Nil(t, result) }) t.Run("Invalid with set githubPushTrigger and not installed github plugin", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -510,7 +510,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.False(t, result) + assert.NotNil(t, result) }) t.Run("Invalid with set githubPushTrigger and not installed github plugin", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -538,7 +538,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.True(t, result) + assert.Nil(t, result) }) } @@ -549,7 +549,7 @@ func TestValidateIfIDIsUnique(t *testing.T) { } ctrl := New(nil, nil, logf.ZapLogger(false)) got := ctrl.validateIfIDIsUnique(seedJobs) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("duplicated ids", func(t *testing.T) { seedJobs := []v1alpha2.SeedJob{ @@ -557,6 +557,6 @@ func TestValidateIfIDIsUnique(t *testing.T) { } ctrl := New(nil, nil, logf.ZapLogger(false)) got := ctrl.validateIfIDIsUnique(seedJobs) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } diff --git a/pkg/controller/jenkins/configuration/user/validate.go b/pkg/controller/jenkins/configuration/user/validate.go index ec86203a..56780bb6 100644 --- a/pkg/controller/jenkins/configuration/user/validate.go +++ b/pkg/controller/jenkins/configuration/user/validate.go @@ -7,10 +7,10 @@ import ( ) // Validate validates Jenkins CR Spec section -func (r *ReconcileUserConfiguration) Validate(jenkins *v1alpha2.Jenkins) (bool, error) { +func (r *ReconcileUserConfiguration) Validate(jenkins *v1alpha2.Jenkins) ([]string, error) { backupAndRestore := backuprestore.New(r.k8sClient, r.clientSet, r.logger, r.jenkins, r.config) - if ok := backupAndRestore.Validate(); !ok { - return false, nil + if msg := backupAndRestore.Validate(); msg != nil { + return msg, nil } seedJobs := seedjobs.New(r.jenkinsClient, r.k8sClient, r.logger) diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 7cf371cb..528fb6d0 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,7 +3,7 @@ package jenkins import ( "context" "fmt" - "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" + "reflect" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" @@ -12,8 +12,8 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base/resources" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/user" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" + "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/plugins" - "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" @@ -35,15 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" ) -const ( - // reasonBaseConfigurationSuccess is the event which informs base configuration has been completed successfully - reasonBaseConfigurationSuccess event.Reason = "BaseConfigurationSuccess" - // reasonUserConfigurationSuccess is the event which informs user configuration has been completed successfully - reasonUserConfigurationSuccess event.Reason = "BaseConfigurationFailure" - // reasonCRValidationFailure is the event which informs user has provided invalid configuration in Jenkins CR - reasonCRValidationFailure event.Reason = "CRValidationFailure" -) - type reconcileError struct { err error counter uint64 @@ -53,18 +44,17 @@ var reconcileErrors = map[string]reconcileError{} // Add creates a new Jenkins Controller and adds it to the Manager. The Manager will set fields on the Controller // and Start it when the Manager is Started. -func Add(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) error { - return add(mgr, newReconciler(mgr, local, minikube, events, clientSet, config, notificationEvents)) +func Add(mgr manager.Manager, local, minikube bool, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) error { + return add(mgr, newReconciler(mgr, local, minikube, clientSet, config, notificationEvents)) } // newReconciler returns a new reconcile.Reconciler -func newReconciler(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) reconcile.Reconciler { +func newReconciler(mgr manager.Manager, local, minikube bool, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) reconcile.Reconciler { return &ReconcileJenkins{ client: mgr.GetClient(), scheme: mgr.GetScheme(), local: local, minikube: minikube, - events: events, clientSet: clientSet, config: config, notificationEvents: notificationEvents, @@ -124,7 +114,6 @@ type ReconcileJenkins struct { client client.Client scheme *runtime.Scheme local, minikube bool - events event.Recorder clientSet kubernetes.Clientset config rest.Config notificationEvents *chan notifications.Event @@ -136,19 +125,6 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger := r.buildLogger(request.Name) logger.V(log.VDebug).Info("Reconciling Jenkins") - jenkins := &v1alpha2.Jenkins{} - err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, errors.WithStack(err) - } - result, err := r.reconcile(request, logger) if err != nil && apierrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil @@ -175,12 +151,25 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %s", reconcileFailLimit, err)) } + jenkins := &v1alpha2.Jenkins{} + err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. + // Return and don't requeue + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, errors.WithStack(err) + } + *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeUnknown, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %s", err), - MessageVerbose: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err), + MessagesVerbose: []string{fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err)}, } return reconcile.Result{Requeue: false}, nil } @@ -219,25 +208,26 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg if err != nil { return reconcile.Result{}, err } - // Reconcile base configuration baseConfiguration := base.New(r.client, r.scheme, logger, jenkins, r.local, r.minikube, &r.clientSet, &r.config) - valid, err := baseConfiguration.Validate(jenkins) + messages, err := baseConfiguration.Validate(jenkins) if err != nil { return reconcile.Result{}, err } - if !valid { - message := "Validation of base configuration failed, please correct Jenkins CR" + if messages != nil { + message := "Validation of base configuration failed, please correct Jenkins CR." *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeBase, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } - r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "Base CR validation failed") logger.V(log.VWarn).Info(message) + for _, msg := range messages { + logger.V(log.VDebug).Info(msg) + } return reconcile.Result{}, nil // don't requeue } @@ -267,29 +257,31 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg ConfigurationType: notifications.ConfigurationTypeBase, LogLevel: v1alpha2.NotificationLogLevelInfo, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } logger.Info(message) - r.events.Emit(jenkins, event.TypeNormal, reasonBaseConfigurationSuccess, "Base configuration completed") } // Reconcile user configuration userConfiguration := user.New(r.client, jenkinsClient, logger, jenkins, r.clientSet, r.config) - valid, err = userConfiguration.Validate(jenkins) + messages, err = userConfiguration.Validate(jenkins) if err != nil { return reconcile.Result{}, err } - if !valid { + if messages != nil { message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeUser, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } + logger.V(log.VWarn).Info(message) - r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "User CR validation failed") + for _, msg := range messages { + logger.V(log.VDebug).Info(msg) + } return reconcile.Result{}, nil // don't requeue } @@ -315,10 +307,9 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg ConfigurationType: notifications.ConfigurationTypeUser, LogLevel: v1alpha2.NotificationLogLevelInfo, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } logger.Info(message) - r.events.Emit(jenkins, event.TypeNormal, reasonUserConfigurationSuccess, "User configuration completed") } return reconcile.Result{}, nil diff --git a/pkg/controller/jenkins/notifications/mailgun.go b/pkg/controller/jenkins/notifications/mailgun.go index 8e705c56..8fd3dab3 100644 --- a/pkg/controller/jenkins/notifications/mailgun.go +++ b/pkg/controller/jenkins/notifications/mailgun.go @@ -73,12 +73,17 @@ func (m MailGun) Send(event Event, config v1alpha2.Notification) error { var statusMessage string if config.Verbose { - statusMessage = event.MessageVerbose + message := event.Message + "
    " + for _, msg := range event.MessagesVerbose { + message = message + "
  • " + msg + "
    • " + } + message = message + "
" + statusMessage = message } else { statusMessage = event.Message } - htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), statusMessage, event.Jenkins.Name, event.ConfigurationType, m.getStatusColor(event.LogLevel)) + htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), notificationTitle(event), statusMessage, event.Jenkins.Name, event.ConfigurationType) msg := mg.NewMessage(fmt.Sprintf("Jenkins Operator Notifier <%s>", config.Mailgun.From), notificationTitle(event), "", config.Mailgun.Recipient) msg.SetHtml(htmlMessage) diff --git a/pkg/controller/jenkins/notifications/msteams.go b/pkg/controller/jenkins/notifications/msteams.go index 76a6747a..daf6817d 100644 --- a/pkg/controller/jenkins/notifications/msteams.go +++ b/pkg/controller/jenkins/notifications/msteams.go @@ -94,14 +94,18 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { tm.Title = notificationTitle(event) if config.Verbose { - tm.Sections[0].Text = event.MessageVerbose - tm.Summary = event.MessageVerbose + message := event.Message + for _, msg := range event.MessagesVerbose { + message = message + "\n\n - " + msg + } + tm.Sections[0].Text += message + tm.Summary = message } if event.ConfigurationType != ConfigurationTypeUnknown { tm.Sections[0].Facts = append(tm.Sections[0].Facts, TeamsFact{ Name: configurationTypeFieldName, - Value: event.ConfigurationType, + Value: string(event.ConfigurationType), }) } diff --git a/pkg/controller/jenkins/notifications/msteams_test.go b/pkg/controller/jenkins/notifications/msteams_test.go index 36ea3cde..b223a4cd 100644 --- a/pkg/controller/jenkins/notifications/msteams_test.go +++ b/pkg/controller/jenkins/notifications/msteams_test.go @@ -29,7 +29,7 @@ func TestTeams_Send(t *testing.T) { }, ConfigurationType: testConfigurationType, Message: testMessage, - MessageVerbose: testMessageVerbose, + MessagesVerbose: testMessageVerbose, LogLevel: testLoggingLevel, } teams := Teams{k8sClient: fakeClient} @@ -52,7 +52,7 @@ func TestTeams_Send(t *testing.T) { for _, fact := range mainSection.Facts { switch fact.Name { case configurationTypeFieldName: - assert.Equal(t, fact.Value, event.ConfigurationType) + assert.Equal(t, fact.Value, string(event.ConfigurationType)) case crNameFieldName: assert.Equal(t, fact.Value, event.Jenkins.Name) case messageFieldName: diff --git a/pkg/controller/jenkins/notifications/sender.go b/pkg/controller/jenkins/notifications/sender.go index 3b12e5bc..0f426530 100644 --- a/pkg/controller/jenkins/notifications/sender.go +++ b/pkg/controller/jenkins/notifications/sender.go @@ -2,10 +2,10 @@ package notifications import ( "fmt" - "net/http" - "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" + "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" + "net/http" "github.com/pkg/errors" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,33 +17,36 @@ const ( messageFieldName = "Message" loggingLevelFieldName = "Logging Level" crNameFieldName = "CR Name" - configurationTypeFieldName = "Configuration Type" + configurationTypeFieldName = "Phase" namespaceFieldName = "Namespace" footerContent = "Powered by Jenkins Operator" ) const ( // ConfigurationTypeBase is core configuration of Jenkins provided by the Operator - ConfigurationTypeBase = "base" + ConfigurationTypeBase ConfigurationType = "base" // ConfigurationTypeUser is user-defined configuration of Jenkins - ConfigurationTypeUser = "user" + ConfigurationTypeUser ConfigurationType = "user" // ConfigurationTypeUnknown is untraceable type of configuration - ConfigurationTypeUnknown = "unknown" + ConfigurationTypeUnknown ConfigurationType = "unknown" ) var ( - testConfigurationType = "test-configuration" + testConfigurationType = ConfigurationTypeUser testCrName = "test-cr" testNamespace = "default" testMessage = "test-message" - testMessageVerbose = "detail-test-message" + testMessageVerbose = []string{"detail-test-message"} testLoggingLevel = v1alpha2.NotificationLogLevelWarning client = http.Client{} ) +// ConfigurationType defines the type of configuration +type ConfigurationType string + // StatusColor is useful for better UX type StatusColor string @@ -53,10 +56,10 @@ type LoggingLevel string // Event contains event details which will be sent as a notification type Event struct { Jenkins v1alpha2.Jenkins - ConfigurationType string + ConfigurationType ConfigurationType LogLevel v1alpha2.NotificationLogLevel Message string - MessageVerbose string + MessagesVerbose []string } type service interface { @@ -64,7 +67,7 @@ type service interface { } // Listen listens for incoming events and send it as notifications -func Listen(events chan Event, k8sClient k8sclient.Client) { +func Listen(events chan Event, k8sEvent event.Recorder, k8sClient k8sclient.Client) { for event := range events { logger := log.Log.WithValues("cr", event.Jenkins.Name) for _, notificationConfig := range event.Jenkins.Spec.Notifications { @@ -94,7 +97,18 @@ func Listen(events chan Event, k8sClient k8sclient.Client) { } }(notificationConfig) } + k8sEvent.Emit(&event.Jenkins, logLevelEventType(event.LogLevel), "NotificationSent", event.Message) + } +} +func logLevelEventType(level v1alpha2.NotificationLogLevel) event.Type { + switch level { + case v1alpha2.NotificationLogLevelWarning: + return event.TypeWarning + case v1alpha2.NotificationLogLevelInfo: + return event.TypeNormal + default: + return event.TypeNormal } } @@ -111,6 +125,6 @@ func notificationTitle(event Event) string { } else if event.LogLevel == v1alpha2.NotificationLogLevelWarning { return warnTitleText } else { - return "undefined" + return "" } } diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index d1081fcb..8db3975e 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -97,13 +97,17 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { if config.Verbose { // TODO: or for title == message - mainAttachment.Fields[0].Value = event.MessageVerbose + message := event.Message + for _, msg := range event.MessagesVerbose { + message = message + "\n - " + msg + } + mainAttachment.Fields[0].Value = message } if event.ConfigurationType != ConfigurationTypeUnknown { mainAttachment.Fields = append(mainAttachment.Fields, SlackField{ Title: configurationTypeFieldName, - Value: event.ConfigurationType, + Value: string(event.ConfigurationType), Short: true, }) } @@ -115,7 +119,7 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { secretValue := string(secret.Data[selector.Key]) if secretValue == "" { - return errors.Errorf("SecretValue %s is empty", selector.Name) + return errors.Errorf("Secret with given name `%s` and selector name `%s` is empty", secret.Name, selector.Name) } request, err := http.NewRequest("POST", secretValue, bytes.NewBuffer(slackMessage)) diff --git a/pkg/controller/jenkins/notifications/slack_test.go b/pkg/controller/jenkins/notifications/slack_test.go index d40b7c71..935eaed2 100644 --- a/pkg/controller/jenkins/notifications/slack_test.go +++ b/pkg/controller/jenkins/notifications/slack_test.go @@ -29,7 +29,7 @@ func TestSlack_Send(t *testing.T) { }, ConfigurationType: testConfigurationType, Message: testMessage, - MessageVerbose: testMessageVerbose, + MessagesVerbose: testMessageVerbose, LogLevel: testLoggingLevel, } slack := Slack{k8sClient: fakeClient} @@ -49,7 +49,7 @@ func TestSlack_Send(t *testing.T) { for _, field := range mainAttachment.Fields { switch field.Title { case configurationTypeFieldName: - assert.Equal(t, field.Value, event.ConfigurationType) + assert.Equal(t, field.Value, string(event.ConfigurationType)) case crNameFieldName: assert.Equal(t, field.Value, event.Jenkins.Name) case messageFieldName: From 4e8b0fa72ebb21c0ce8daebbce3034b63f8d91f3 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Thu, 3 Oct 2019 15:53:39 +0200 Subject: [PATCH 12/16] Improved notification tests, refactor ConfigurationType to Phase --- .../jenkins/configuration/base/validate.go | 45 ++++---- .../configuration/base/validate_test.go | 55 +++++---- .../configuration/user/seedjobs/validate.go | 57 +++++----- .../user/seedjobs/validate_test.go | 42 ++++--- pkg/controller/jenkins/jenkins_controller.go | 105 ++++++++---------- .../jenkins/notifications/mailgun.go | 4 +- .../jenkins/notifications/msteams.go | 6 +- .../jenkins/notifications/msteams_test.go | 12 +- .../jenkins/notifications/sender.go | 58 +++++----- pkg/controller/jenkins/notifications/slack.go | 6 +- .../jenkins/notifications/slack_test.go | 12 +- pkg/controller/jenkins/plugins/plugin.go | 11 +- pkg/controller/jenkins/plugins/plugin_test.go | 16 +-- 13 files changed, 222 insertions(+), 207 deletions(-) diff --git a/pkg/controller/jenkins/configuration/base/validate.go b/pkg/controller/jenkins/configuration/base/validate.go index 47c4df7b..676f8d8f 100644 --- a/pkg/controller/jenkins/configuration/base/validate.go +++ b/pkg/controller/jenkins/configuration/base/validate.go @@ -6,13 +6,11 @@ import ( "regexp" "strings" + docker "github.com/docker/distribution/reference" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base/resources" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/plugins" - "github.com/jenkinsci/kubernetes-operator/pkg/log" - - docker "github.com/docker/distribution/reference" stackerr "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -27,38 +25,38 @@ var ( func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) ([]string, error) { var messages []string - if msg := r.validateReservedVolumes(); msg != nil { + if msg := r.validateReservedVolumes(); len(msg) > 0 { messages = append(messages, msg...) } if msg, err := r.validateVolumes(); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } for _, container := range jenkins.Spec.Master.Containers { - if msg := r.validateContainer(container); msg != nil { + if msg := r.validateContainer(container); len(msg) > 0 { messages = append(messages, msg...) } } - if msg := r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins); msg != nil { + if msg := r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins); len(msg) > 0 { messages = append(messages, msg...) } - if msg := r.validateJenkinsMasterPodEnvs(); msg != nil { + if msg := r.validateJenkinsMasterPodEnvs(); len(msg) > 0 { messages = append(messages, msg...) } if msg, err := r.validateCustomization(r.jenkins.Spec.GroovyScripts.Customization, "spec.groovyScripts"); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } if msg, err := r.validateCustomization(r.jenkins.Spec.ConfigurationAsCode.Customization, "spec.configurationAsCode"); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } @@ -72,7 +70,7 @@ func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecrets() ([]string if err != nil { return nil, err } - if msg != nil { + if len(msg) > 0 { messages = append(messages, msg...) } } @@ -112,19 +110,19 @@ func (r *ReconcileJenkinsBaseConfiguration) validateVolumes() ([]string, error) case volume.ConfigMap != nil: if msg, err := r.validateConfigMapVolume(volume); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } case volume.Secret != nil: if msg, err := r.validateSecretVolume(volume); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } case volume.PersistentVolumeClaim != nil: if msg, err := r.validatePersistentVolumeClaim(volume); err != nil { return nil, err - } else if msg != nil { + } else if len(msg) > 0 { messages = append(messages, msg...) } default: //TODO add support for rest of volumes @@ -211,7 +209,7 @@ func (r *ReconcileJenkinsBaseConfiguration) validateContainer(container v1alpha2 messages = append(messages, "Image pull policy not set") } - if msg := r.validateContainerVolumeMounts(container); msg != nil { + if msg := r.validateContainerVolumeMounts(container); len(msg) > 0 { messages = append(messages, msg...) } @@ -307,19 +305,19 @@ func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins } } - if !plugins.VerifyDependencies(allPlugins) { - messages = append(messages, "Invalid dependencies") + if msg := plugins.VerifyDependencies(allPlugins); len(msg) > 0 { + messages = append(messages, msg...) } - if !r.verifyBasePlugins(requiredBasePlugins, basePlugins) { - messages = append(messages, "Failed to verify base plugins") + if msg := r.verifyBasePlugins(requiredBasePlugins, basePlugins); len(msg) > 0 { + messages = append(messages, msg...) } return messages } -func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugins []plugins.Plugin, basePlugins []v1alpha2.Plugin) bool { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugins []plugins.Plugin, basePlugins []v1alpha2.Plugin) []string { + var messages []string for _, requiredBasePlugin := range requiredBasePlugins { found := false @@ -330,12 +328,11 @@ func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugin } } if !found { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Missing plugin '%s' in spec.master.basePlugins", requiredBasePlugin.Name)) + messages = append(messages, fmt.Sprintf("Missing plugin '%s' in spec.master.basePlugins", requiredBasePlugin.Name)) } } - return valid + return messages } func (r *ReconcileJenkinsBaseConfiguration) validateCustomization(customization v1alpha2.Customization, name string) ([]string, error) { diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index ca231a73..3a275e8c 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -48,7 +48,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"invalid plugin name 'INVALID?:0.0.1', must follow pattern '(?i)^[0-9a-z-_]+$'"}) }) t.Run("invalid user plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -57,7 +57,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"invalid plugin version 'simple-plugin:invalid', must follow pattern '^[0-9\\\\.]+$'"}) }) t.Run("valid base plugin", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -75,7 +75,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"invalid plugin name 'INVALID?:0.0.1', must follow pattern '(?i)^[0-9a-z-_]+$'"}) }) t.Run("invalid base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -84,7 +84,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"invalid plugin version 'simple-plugin:invalid', must follow pattern '^[0-9\\\\.]+$'"}) }) t.Run("valid user and base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -129,7 +129,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"Missing plugin 'simple-plugin' in spec.master.basePlugins"}) }) } @@ -186,7 +186,8 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret test-ref not found defined in spec.master.imagePullSecrets", "Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key.", "Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key.", "Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key.", "Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key."}) }) t.Run("no docker email", func(t *testing.T) { @@ -219,7 +220,8 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key."}) }) t.Run("no docker password", func(t *testing.T) { @@ -252,7 +254,8 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key."}) }) t.Run("no docker username", func(t *testing.T) { @@ -285,7 +288,8 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key."}) }) t.Run("no docker server", func(t *testing.T) { @@ -318,7 +322,8 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'test-ref' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key."}) }) } @@ -374,7 +379,8 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Jenkins Master container env 'JENKINS_HOME' cannot be overridden"}) }) t.Run("missing -Djava.awt.headless=true in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -396,7 +402,8 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Jenkins Master container env 'JAVA_OPTS' doesn't have required flag '-Djava.awt.headless=true'"}) }) t.Run("missing -Djenkins.install.runSetupWizard=false in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -418,7 +425,8 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Jenkins Master container env 'JAVA_OPTS' doesn't have required flag '-Djenkins.install.runSetupWizard=false'"}) }) } @@ -455,7 +463,8 @@ func TestValidateReservedVolumes(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateReservedVolumes() - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Jenkins Master pod volume 'jenkins-home' is reserved please choose different one"}) }) } @@ -545,7 +554,8 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Not found volume for 'missing-volume' volume mount in container ''"}) }) } @@ -618,7 +628,8 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"ConfigMap 'configmap-name' not found for volume '{volume-name {nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil &ConfigMapVolumeSource{LocalObjectReference:LocalObjectReference{Name:configmap-name,},Items:[],DefaultMode:nil,Optional:*false,} nil nil nil nil nil nil nil nil}}'"}) }) } @@ -687,7 +698,8 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'secret-name' not found for volume '{volume-name {nil nil nil nil nil &SecretVolumeSource{SecretName:secret-name,Items:[],DefaultMode:nil,Optional:*false,} nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil}}'"}) }) } @@ -731,7 +743,8 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"spec.groovyScripts.secret.name is set but spec.groovyScripts.configurations is empty"}) }) t.Run("secret and configmap exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -784,7 +797,8 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"Secret 'secretName' configured in spec.groovyScripts.secret.name not found"}) }) t.Run("secret exists and configmap not exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -806,6 +820,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"ConfigMap 'configmap-name' configured in spec.groovyScripts.configurations[0] not found"}) }) } diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go index d97dfd56..15798637 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go @@ -8,9 +8,6 @@ import ( "strings" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" - "github.com/jenkinsci/kubernetes-operator/pkg/log" - - "github.com/go-logr/logr" stackerr "github.com/pkg/errors" "github.com/robfig/cron" "k8s.io/api/core/v1" @@ -22,41 +19,39 @@ import ( func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) ([]string, error) { var messages []string - if msg := s.validateIfIDIsUnique(jenkins.Spec.SeedJobs); msg != nil { + if msg := s.validateIfIDIsUnique(jenkins.Spec.SeedJobs); len(msg) > 0 { messages = append(messages, msg...) } for _, seedJob := range jenkins.Spec.SeedJobs { - logger := s.logger.WithValues("seedJob", seedJob.ID).V(log.VWarn) - if len(seedJob.ID) == 0 { - messages = append(messages, "id can't be empty") + messages = append(messages, fmt.Sprintf("seedJob `%s` id can't be empty", seedJob.ID)) } if len(seedJob.RepositoryBranch) == 0 { - messages = append(messages, "repository branch can't be empty") + messages = append(messages, fmt.Sprintf("seedJob `%s` repository branch can't be empty", seedJob.ID)) } if len(seedJob.RepositoryURL) == 0 { - messages = append(messages, "repository URL branch can't be empty") + messages = append(messages, fmt.Sprintf("seedJob `%s` repository URL branch can't be empty", seedJob.ID)) } if len(seedJob.Targets) == 0 { - messages = append(messages, "targets can't be empty") + messages = append(messages, fmt.Sprintf("seedJob `%s` targets can't be empty", seedJob.ID)) } if _, ok := v1alpha2.AllowedJenkinsCredentialMap[string(seedJob.JenkinsCredentialType)]; !ok { - messages = append(messages, "unknown credential type") + messages = append(messages, fmt.Sprintf("seedJob `%s` unknown credential type", seedJob.ID)) } if (seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType) && len(seedJob.CredentialID) == 0 { - messages = append(messages, "credential ID can't be empty") + messages = append(messages, fmt.Sprintf("seedJob `%s` credential ID can't be empty", seedJob.ID)) } // validate repository url match private key if strings.Contains(seedJob.RepositoryURL, "git@") && seedJob.JenkinsCredentialType == v1alpha2.NoJenkinsCredentialCredentialType { - messages = append(messages, "Jenkins credential must be set while using ssh repository url") + messages = append(messages, fmt.Sprintf("seedJob `%s` Jenkins credential must be set while using ssh repository url", seedJob.ID)) } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { @@ -64,38 +59,48 @@ func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) ([]string, error) namespaceName := types.NamespacedName{Namespace: jenkins.Namespace, Name: seedJob.CredentialID} err := s.k8sClient.Get(context.TODO(), namespaceName, secret) if err != nil && apierrors.IsNotFound(err) { - messages = append(messages, fmt.Sprintf("required secret '%s' with Jenkins credential not found", seedJob.CredentialID)) + messages = append(messages, fmt.Sprintf("seedJob `%s` required secret '%s' with Jenkins credential not found", seedJob.ID, seedJob.CredentialID)) } else if err != nil { return nil, stackerr.WithStack(err) } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType { - if msg := validateBasicSSHSecret(logger, *secret); msg != nil { - messages = append(messages, msg...) + if msg := validateBasicSSHSecret(*secret); len(msg) > 0 { + for _, m := range msg { + messages = append(messages, fmt.Sprintf("seedJob `%s` %s", seedJob.ID, m)) + } } } if seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { - if msg := validateUsernamePasswordSecret(logger, *secret); msg != nil { - messages = append(messages, msg...) + if msg := validateUsernamePasswordSecret(*secret); len(msg) > 0 { + for _, m := range msg { + messages = append(messages, fmt.Sprintf("seedJob `%s` %s", seedJob.ID, m)) + } } } } if len(seedJob.BuildPeriodically) > 0 { - if msg := s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically"); msg != nil { - messages = append(messages, msg...) + if msg := s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically"); len(msg) > 0 { + for _, m := range msg { + messages = append(messages, fmt.Sprintf("seedJob `%s` %s", seedJob.ID, m)) + } } } if len(seedJob.PollSCM) > 0 { - if msg := s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM"); msg != nil { - messages = append(messages, msg...) + if msg := s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM"); len(msg) > 0 { + for _, m := range msg { + messages = append(messages, fmt.Sprintf("seedJob `%s` %s", seedJob.ID, m)) + } } } if seedJob.GitHubPushTrigger { - if msg := s.validateGitHubPushTrigger(jenkins); msg != nil { - messages = append(messages, msg...) + if msg := s.validateGitHubPushTrigger(jenkins); len(msg) > 0 { + for _, m := range msg { + messages = append(messages, fmt.Sprintf("seedJob `%s` %s", seedJob.ID, m)) + } } } } @@ -146,7 +151,7 @@ func (s *SeedJobs) validateIfIDIsUnique(seedJobs []v1alpha2.SeedJob) []string { return messages } -func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) []string { +func validateBasicSSHSecret(secret v1.Secret) []string { var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { @@ -170,7 +175,7 @@ func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) []string { return messages } -func validateUsernamePasswordSecret(logger logr.InfoLogger, secret v1.Secret) []string { +func validateUsernamePasswordSecret(secret v1.Secret) []string { var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go index 707db9a8..e1d75f20 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go @@ -96,7 +96,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `` id can't be empty"}) }) t.Run("Valid with private key and secret", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -162,7 +163,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` private key 'privateKey' invalid in secret 'deploy-keys': failed to decode PEM block"}) }) t.Run("Invalid with PrivateKey and empty Secret data", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -195,7 +197,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` required data 'privateKey' not found in secret 'deploy-keys'", "seedJob `example` private key 'privateKey' invalid in secret 'deploy-keys': failed to decode PEM block"}) }) t.Run("Invalid with ssh RepositoryURL and empty PrivateKey", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -217,7 +220,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string([]string{"seedJob `example` required secret 'jenkins-operator-e2e' with Jenkins credential not found", "seedJob `example` required data 'username' not found in secret ''", "seedJob `example` required data 'username' is empty in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` private key 'privateKey' invalid in secret '': failed to decode PEM block"})) }) t.Run("Invalid without targets", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -237,7 +241,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` targets can't be empty"}) }) t.Run("Invalid without repository URL", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -257,7 +262,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` repository URL branch can't be empty"}) }) t.Run("Invalid without repository branch", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -277,7 +283,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` repository branch can't be empty"}) }) t.Run("Valid with username and password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -343,7 +350,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` required data 'username' is empty in secret 'deploy-keys'"}) }) t.Run("Invalid with empty password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -376,7 +384,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` required data 'password' is empty in secret 'deploy-keys'"}) }) t.Run("Invalid without username", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -408,7 +417,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` required data 'username' not found in secret 'deploy-keys'", "seedJob `example` required data 'username' is empty in secret 'deploy-keys'"}) }) t.Run("Invalid without password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -440,7 +450,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` required data 'password' not found in secret 'deploy-keys'", "seedJob `example` required data 'password' is empty in secret 'deploy-keys'"}) }) t.Run("Invalid with wrong cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -463,7 +474,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` `buildPeriodically` schedule 'invalid-cron-spec' is invalid cron spec in `example`"}) }) t.Run("Valid with good cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -510,7 +522,8 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.NotNil(t, result) + + assert.Equal(t, result, []string{"seedJob `example` githubPushTrigger is set. This function requires `github` plugin installed in .Spec.Master.Plugins because seed jobs Push Trigger function needs it"}) }) t.Run("Invalid with set githubPushTrigger and not installed github plugin", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -557,6 +570,7 @@ func TestValidateIfIDIsUnique(t *testing.T) { } ctrl := New(nil, nil, logf.ZapLogger(false)) got := ctrl.validateIfIDIsUnique(seedJobs) - assert.NotNil(t, got) + + assert.Equal(t, got, []string{"'first' seed job ID is not unique"}) }) } diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 528fb6d0..0c085fdd 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -125,7 +125,7 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger := r.buildLogger(request.Name) logger.V(log.VDebug).Info("Reconciling Jenkins") - result, err := r.reconcile(request, logger) + result, jenkins, err := r.reconcile(request, logger) if err != nil && apierrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil } else if err != nil { @@ -151,25 +151,12 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %s", reconcileFailLimit, err)) } - jenkins := &v1alpha2.Jenkins{} - err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, errors.WithStack(err) - } - *r.notificationEvents <- notifications.Event{ - Jenkins: *jenkins, - ConfigurationType: notifications.ConfigurationTypeUnknown, - LogLevel: v1alpha2.NotificationLogLevelWarning, - Message: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %s", err), - MessagesVerbose: []string{fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err)}, + Jenkins: *jenkins, + Phase: notifications.PhaseBase, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %s", reconcileFailLimit, err), + MessagesVerbose: []string{fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %+v", reconcileFailLimit, err)}, } return reconcile.Result{Requeue: false}, nil } @@ -190,7 +177,7 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul return result, nil } -func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logger) (reconcile.Result, error) { +func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logger) (reconcile.Result, *v1alpha2.Jenkins, error) { // Fetch the Jenkins instance jenkins := &v1alpha2.Jenkins{} err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) @@ -199,47 +186,47 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg // Request object not found, could have been deleted after reconcile request. // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. // Return and don't requeue - return reconcile.Result{}, nil + return reconcile.Result{}, nil, nil } // Error reading the object - requeue the request. - return reconcile.Result{}, errors.WithStack(err) + return reconcile.Result{}, nil, errors.WithStack(err) } err = r.setDefaults(jenkins, logger) if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, nil, err } // Reconcile base configuration baseConfiguration := base.New(r.client, r.scheme, logger, jenkins, r.local, r.minikube, &r.clientSet, &r.config) messages, err := baseConfiguration.Validate(jenkins) if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, nil, err } - if messages != nil { + if len(messages) > 0 { message := "Validation of base configuration failed, please correct Jenkins CR." *r.notificationEvents <- notifications.Event{ - Jenkins: *jenkins, - ConfigurationType: notifications.ConfigurationTypeBase, - LogLevel: v1alpha2.NotificationLogLevelWarning, - Message: message, - MessagesVerbose: messages, + Jenkins: *jenkins, + Phase: notifications.PhaseBase, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: message, + MessagesVerbose: messages, } logger.V(log.VWarn).Info(message) for _, msg := range messages { - logger.V(log.VDebug).Info(msg) + logger.V(log.VWarn).Info(msg) } - return reconcile.Result{}, nil // don't requeue + return reconcile.Result{}, nil, nil // don't requeue } result, jenkinsClient, err := baseConfiguration.Reconcile() if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, nil, err } if result.Requeue { - return result, nil + return result, nil, nil } if jenkinsClient == nil { - return reconcile.Result{Requeue: false}, nil + return reconcile.Result{Requeue: false}, nil, nil } if jenkins.Status.BaseConfigurationCompletedTime == nil { @@ -247,17 +234,17 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg jenkins.Status.BaseConfigurationCompletedTime = &now err = r.client.Update(context.TODO(), jenkins) if err != nil { - return reconcile.Result{}, errors.WithStack(err) + return reconcile.Result{}, nil, errors.WithStack(err) } message := fmt.Sprintf("Base configuration phase is complete, took %s", jenkins.Status.BaseConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time)) *r.notificationEvents <- notifications.Event{ - Jenkins: *jenkins, - ConfigurationType: notifications.ConfigurationTypeBase, - LogLevel: v1alpha2.NotificationLogLevelInfo, - Message: message, - MessagesVerbose: messages, + Jenkins: *jenkins, + Phase: notifications.PhaseBase, + LogLevel: v1alpha2.NotificationLogLevelInfo, + Message: message, + MessagesVerbose: messages, } logger.Info(message) } @@ -266,31 +253,31 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg messages, err = userConfiguration.Validate(jenkins) if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, nil, err } - if messages != nil { + if len(messages) > 0 { message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") *r.notificationEvents <- notifications.Event{ - Jenkins: *jenkins, - ConfigurationType: notifications.ConfigurationTypeUser, - LogLevel: v1alpha2.NotificationLogLevelWarning, - Message: message, - MessagesVerbose: messages, + Jenkins: *jenkins, + Phase: notifications.PhaseUser, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: message, + MessagesVerbose: messages, } logger.V(log.VWarn).Info(message) for _, msg := range messages { - logger.V(log.VDebug).Info(msg) + logger.V(log.VWarn).Info(msg) } - return reconcile.Result{}, nil // don't requeue + return reconcile.Result{}, nil, nil // don't requeue } result, err = userConfiguration.Reconcile() if err != nil { - return reconcile.Result{}, err + return reconcile.Result{}, nil, err } if result.Requeue { - return result, nil + return result, nil, nil } if jenkins.Status.UserConfigurationCompletedTime == nil { @@ -298,21 +285,21 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg jenkins.Status.UserConfigurationCompletedTime = &now err = r.client.Update(context.TODO(), jenkins) if err != nil { - return reconcile.Result{}, errors.WithStack(err) + return reconcile.Result{}, nil, errors.WithStack(err) } message := fmt.Sprintf("User configuration phase is complete, took %s", jenkins.Status.UserConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time)) *r.notificationEvents <- notifications.Event{ - Jenkins: *jenkins, - ConfigurationType: notifications.ConfigurationTypeUser, - LogLevel: v1alpha2.NotificationLogLevelInfo, - Message: message, - MessagesVerbose: messages, + Jenkins: *jenkins, + Phase: notifications.PhaseUser, + LogLevel: v1alpha2.NotificationLogLevelInfo, + Message: message, + MessagesVerbose: messages, } logger.Info(message) } - return reconcile.Result{}, nil + return reconcile.Result{}, jenkins, nil } func (r *ReconcileJenkins) buildLogger(jenkinsName string) logr.Logger { diff --git a/pkg/controller/jenkins/notifications/mailgun.go b/pkg/controller/jenkins/notifications/mailgun.go index 8fd3dab3..45a166a2 100644 --- a/pkg/controller/jenkins/notifications/mailgun.go +++ b/pkg/controller/jenkins/notifications/mailgun.go @@ -28,7 +28,7 @@ const content = ` %s - Configuration type: + Phase: %s @@ -83,7 +83,7 @@ func (m MailGun) Send(event Event, config v1alpha2.Notification) error { statusMessage = event.Message } - htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), notificationTitle(event), statusMessage, event.Jenkins.Name, event.ConfigurationType) + htmlMessage := fmt.Sprintf(content, m.getStatusColor(event.LogLevel), notificationTitle(event), statusMessage, event.Jenkins.Name, event.Phase) msg := mg.NewMessage(fmt.Sprintf("Jenkins Operator Notifier <%s>", config.Mailgun.From), notificationTitle(event), "", config.Mailgun.Recipient) msg.SetHtml(htmlMessage) diff --git a/pkg/controller/jenkins/notifications/msteams.go b/pkg/controller/jenkins/notifications/msteams.go index daf6817d..c883fbfb 100644 --- a/pkg/controller/jenkins/notifications/msteams.go +++ b/pkg/controller/jenkins/notifications/msteams.go @@ -102,10 +102,10 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { tm.Summary = message } - if event.ConfigurationType != ConfigurationTypeUnknown { + if event.Phase != PhaseUnknown { tm.Sections[0].Facts = append(tm.Sections[0].Facts, TeamsFact{ - Name: configurationTypeFieldName, - Value: string(event.ConfigurationType), + Name: phaseFieldName, + Value: string(event.Phase), }) } diff --git a/pkg/controller/jenkins/notifications/msteams_test.go b/pkg/controller/jenkins/notifications/msteams_test.go index b223a4cd..088f6928 100644 --- a/pkg/controller/jenkins/notifications/msteams_test.go +++ b/pkg/controller/jenkins/notifications/msteams_test.go @@ -27,10 +27,10 @@ func TestTeams_Send(t *testing.T) { Namespace: testNamespace, }, }, - ConfigurationType: testConfigurationType, - Message: testMessage, - MessagesVerbose: testMessageVerbose, - LogLevel: testLoggingLevel, + Phase: testPhase, + Message: testMessage, + MessagesVerbose: testMessageVerbose, + LogLevel: testLoggingLevel, } teams := Teams{k8sClient: fakeClient} @@ -51,8 +51,8 @@ func TestTeams_Send(t *testing.T) { for _, fact := range mainSection.Facts { switch fact.Name { - case configurationTypeFieldName: - assert.Equal(t, fact.Value, string(event.ConfigurationType)) + case phaseFieldName: + assert.Equal(t, fact.Value, string(event.Phase)) case crNameFieldName: assert.Equal(t, fact.Value, event.Jenkins.Name) case messageFieldName: diff --git a/pkg/controller/jenkins/notifications/sender.go b/pkg/controller/jenkins/notifications/sender.go index 0f426530..441d9030 100644 --- a/pkg/controller/jenkins/notifications/sender.go +++ b/pkg/controller/jenkins/notifications/sender.go @@ -2,50 +2,50 @@ package notifications import ( "fmt" + "net/http" + "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" - "net/http" - "github.com/pkg/errors" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( - infoTitleText = "Jenkins Operator reconciliation info" - warnTitleText = "Jenkins Operator reconciliation warning" - messageFieldName = "Message" - loggingLevelFieldName = "Logging Level" - crNameFieldName = "CR Name" - configurationTypeFieldName = "Phase" - namespaceFieldName = "Namespace" - footerContent = "Powered by Jenkins Operator" + infoTitleText = "Jenkins Operator reconciliation info" + warnTitleText = "Jenkins Operator reconciliation warning" + messageFieldName = "Message" + loggingLevelFieldName = "Logging Level" + crNameFieldName = "CR Name" + phaseFieldName = "Phase" + namespaceFieldName = "Namespace" + footerContent = "Powered by Jenkins Operator" ) const ( - // ConfigurationTypeBase is core configuration of Jenkins provided by the Operator - ConfigurationTypeBase ConfigurationType = "base" + // PhaseBase is core configuration of Jenkins provided by the Operator + PhaseBase Phase = "base" - // ConfigurationTypeUser is user-defined configuration of Jenkins - ConfigurationTypeUser ConfigurationType = "user" + // PhaseUser is user-defined configuration of Jenkins + PhaseUser Phase = "user" - // ConfigurationTypeUnknown is untraceable type of configuration - ConfigurationTypeUnknown ConfigurationType = "unknown" + // PhaseUnknown is untraceable type of configuration + PhaseUnknown Phase = "unknown" ) var ( - testConfigurationType = ConfigurationTypeUser - testCrName = "test-cr" - testNamespace = "default" - testMessage = "test-message" - testMessageVerbose = []string{"detail-test-message"} - testLoggingLevel = v1alpha2.NotificationLogLevelWarning + testPhase = PhaseUser + testCrName = "test-cr" + testNamespace = "default" + testMessage = "test-message" + testMessageVerbose = []string{"detail-test-message"} + testLoggingLevel = v1alpha2.NotificationLogLevelWarning client = http.Client{} ) -// ConfigurationType defines the type of configuration -type ConfigurationType string +// Phase defines the type of configuration +type Phase string // StatusColor is useful for better UX type StatusColor string @@ -55,11 +55,11 @@ type LoggingLevel string // Event contains event details which will be sent as a notification type Event struct { - Jenkins v1alpha2.Jenkins - ConfigurationType ConfigurationType - LogLevel v1alpha2.NotificationLogLevel - Message string - MessagesVerbose []string + Jenkins v1alpha2.Jenkins + Phase Phase + LogLevel v1alpha2.NotificationLogLevel + Message string + MessagesVerbose []string } type service interface { diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index 8db3975e..5ee08a98 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -104,10 +104,10 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { mainAttachment.Fields[0].Value = message } - if event.ConfigurationType != ConfigurationTypeUnknown { + if event.Phase != PhaseUnknown { mainAttachment.Fields = append(mainAttachment.Fields, SlackField{ - Title: configurationTypeFieldName, - Value: string(event.ConfigurationType), + Title: phaseFieldName, + Value: string(event.Phase), Short: true, }) } diff --git a/pkg/controller/jenkins/notifications/slack_test.go b/pkg/controller/jenkins/notifications/slack_test.go index 935eaed2..261a6172 100644 --- a/pkg/controller/jenkins/notifications/slack_test.go +++ b/pkg/controller/jenkins/notifications/slack_test.go @@ -27,10 +27,10 @@ func TestSlack_Send(t *testing.T) { Namespace: testNamespace, }, }, - ConfigurationType: testConfigurationType, - Message: testMessage, - MessagesVerbose: testMessageVerbose, - LogLevel: testLoggingLevel, + Phase: testPhase, + Message: testMessage, + MessagesVerbose: testMessageVerbose, + LogLevel: testLoggingLevel, } slack := Slack{k8sClient: fakeClient} @@ -48,8 +48,8 @@ func TestSlack_Send(t *testing.T) { assert.Equal(t, mainAttachment.Title, notificationTitle(event)) for _, field := range mainAttachment.Fields { switch field.Title { - case configurationTypeFieldName: - assert.Equal(t, field.Value, string(event.ConfigurationType)) + case phaseFieldName: + assert.Equal(t, field.Value, string(event.Phase)) case crNameFieldName: assert.Equal(t, field.Value, event.Jenkins.Name) case messageFieldName: diff --git a/pkg/controller/jenkins/plugins/plugin.go b/pkg/controller/jenkins/plugins/plugin.go index ef96fa8a..d4ee39e0 100644 --- a/pkg/controller/jenkins/plugins/plugin.go +++ b/pkg/controller/jenkins/plugins/plugin.go @@ -5,8 +5,6 @@ import ( "regexp" "strings" - "github.com/jenkinsci/kubernetes-operator/pkg/log" - "github.com/pkg/errors" ) @@ -77,10 +75,10 @@ func Must(plugin *Plugin, err error) Plugin { } // VerifyDependencies checks if all plugins have compatible versions -func VerifyDependencies(values ...map[Plugin][]Plugin) bool { +func VerifyDependencies(values ...map[Plugin][]Plugin) []string { + var messages []string // key - plugin name, value array of versions allPlugins := make(map[string][]Plugin) - valid := true for _, value := range values { for rootPlugin, plugins := range value { @@ -105,18 +103,17 @@ func VerifyDependencies(values ...map[Plugin][]Plugin) bool { for _, firstVersion := range versions { for _, secondVersion := range versions { if firstVersion.Version != secondVersion.Version { - log.Log.V(log.VWarn).Info(fmt.Sprintf("Plugin '%s' requires version '%s' but plugin '%s' requires '%s' for plugin '%s'", + messages = append(messages, fmt.Sprintf("Plugin '%s' requires version '%s' but plugin '%s' requires '%s' for plugin '%s'", firstVersion.rootPluginNameAndVersion, firstVersion.Version, secondVersion.rootPluginNameAndVersion, secondVersion.Version, pluginName, )) - valid = false } } } } - return valid + return messages } diff --git a/pkg/controller/jenkins/plugins/plugin_test.go b/pkg/controller/jenkins/plugins/plugin_test.go index 2e050a0e..a4002ddc 100644 --- a/pkg/controller/jenkins/plugins/plugin_test.go +++ b/pkg/controller/jenkins/plugins/plugin_test.go @@ -18,7 +18,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("happy, two root plugins with one depended plugin with the same version", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -30,7 +30,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("happy, two plugin names with names with underscores", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -42,7 +42,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("happy, two plugin names with uppercase names", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -54,7 +54,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("fail, two root plugins have different versions", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -66,7 +66,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("happy, no version collision with two sperate plugins lists", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -80,7 +80,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins, extraPlugins) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("fail, dependent plugins have different versions", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -92,7 +92,7 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("fail, root and dependent plugins have different versions", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -106,6 +106,6 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins, extraPlugins) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } From 2b2095c2d155a81a2d2abba42c715489b2886212 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Fri, 4 Oct 2019 10:13:52 +0200 Subject: [PATCH 13/16] Improve notification tests, add container name to notifications --- .../jenkins/configuration/base/validate.go | 4 +++- .../jenkins/configuration/base/validate_test.go | 5 +++-- .../configuration/user/seedjobs/validate_test.go | 2 +- pkg/controller/jenkins/plugins/plugin_test.go | 13 ++++++++++--- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/controller/jenkins/configuration/base/validate.go b/pkg/controller/jenkins/configuration/base/validate.go index 676f8d8f..4f056ac9 100644 --- a/pkg/controller/jenkins/configuration/base/validate.go +++ b/pkg/controller/jenkins/configuration/base/validate.go @@ -37,7 +37,9 @@ func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) for _, container := range jenkins.Spec.Master.Containers { if msg := r.validateContainer(container); len(msg) > 0 { - messages = append(messages, msg...) + for _, m := range msg { + messages = append(messages, fmt.Sprintf("Container `%s` - %s", container.Name, m)) + } } } diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index 3a275e8c..ea03a478 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -102,7 +102,8 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.NotNil(t, got) + assert.Contains(t, got, "Plugin 'simple-plugin:0.0.1' requires version '0.0.1' but plugin 'simple-plugin:0.0.2' requires '0.0.2' for plugin 'simple-plugin'") + assert.Contains(t, got, "Plugin 'simple-plugin:0.0.2' requires version '0.0.2' but plugin 'simple-plugin:0.0.1' requires '0.0.1' for plugin 'simple-plugin'") }) t.Run("required base plugin set with the same version", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} @@ -532,7 +533,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.NotNil(t, got) + assert.Equal(t, got, []string{"mountPath not set for 'example' volume mount in container ''"}) }) t.Run("missing volume", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go index e1d75f20..1251d184 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go @@ -221,7 +221,7 @@ func TestValidateSeedJobs(t *testing.T) { assert.NoError(t, err) - assert.Equal(t, result, []string([]string{"seedJob `example` required secret 'jenkins-operator-e2e' with Jenkins credential not found", "seedJob `example` required data 'username' not found in secret ''", "seedJob `example` required data 'username' is empty in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` private key 'privateKey' invalid in secret '': failed to decode PEM block"})) + assert.Equal(t, result, []string{"seedJob `example` required secret 'jenkins-operator-e2e' with Jenkins credential not found", "seedJob `example` required data 'username' not found in secret ''", "seedJob `example` required data 'username' is empty in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` required data 'privateKey' not found in secret ''", "seedJob `example` private key 'privateKey' invalid in secret '': failed to decode PEM block"}) }) t.Run("Invalid without targets", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ diff --git a/pkg/controller/jenkins/plugins/plugin_test.go b/pkg/controller/jenkins/plugins/plugin_test.go index a4002ddc..26116a67 100644 --- a/pkg/controller/jenkins/plugins/plugin_test.go +++ b/pkg/controller/jenkins/plugins/plugin_test.go @@ -66,7 +66,8 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.NotNil(t, got) + assert.Contains(t, got, "Plugin 'first-root-plugin:1.0.0' requires version '1.0.0' but plugin 'first-root-plugin:2.0.0' requires '2.0.0' for plugin 'first-root-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:2.0.0' requires version '2.0.0' but plugin 'first-root-plugin:1.0.0' requires '1.0.0' for plugin 'first-root-plugin'") }) t.Run("happy, no version collision with two sperate plugins lists", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -92,7 +93,10 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins) - assert.NotNil(t, got) + assert.Contains(t, got, "Plugin 'first-root-plugin:1.0.0' requires version '1.0.0' but plugin 'first-root-plugin:2.0.0' requires '2.0.0' for plugin 'first-root-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:2.0.0' requires version '2.0.0' but plugin 'first-root-plugin:1.0.0' requires '1.0.0' for plugin 'first-root-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:1.0.0' requires version '0.0.1' but plugin 'first-root-plugin:2.0.0' requires '0.0.2' for plugin 'first-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:2.0.0' requires version '0.0.2' but plugin 'first-root-plugin:1.0.0' requires '0.0.1' for plugin 'first-plugin'") }) t.Run("fail, root and dependent plugins have different versions", func(t *testing.T) { basePlugins := map[Plugin][]Plugin{ @@ -106,6 +110,9 @@ func TestVerifyDependencies(t *testing.T) { }, } got := VerifyDependencies(basePlugins, extraPlugins) - assert.NotNil(t, got) + assert.Contains(t, got, "Plugin 'first-root-plugin:1.0.0' requires version '1.0.0' but plugin 'first-root-plugin:2.0.0' requires '2.0.0' for plugin 'first-root-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:2.0.0' requires version '2.0.0' but plugin 'first-root-plugin:1.0.0' requires '1.0.0' for plugin 'first-root-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:1.0.0' requires version '0.0.1' but plugin 'first-root-plugin:2.0.0' requires '0.0.2' for plugin 'first-plugin'") + assert.Contains(t, got, "Plugin 'first-root-plugin:2.0.0' requires version '0.0.2' but plugin 'first-root-plugin:1.0.0' requires '0.0.1' for plugin 'first-plugin'") }) } From 9e882d585327d793efeca3d8ccb9c1c25975b2cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20S=C4=99k?= Date: Fri, 4 Oct 2019 17:34:41 +0200 Subject: [PATCH 14/16] Improve notifications --- go.mod | 4 +- go.sum | 4 ++ pkg/controller/jenkins/client/script.go | 7 ++- .../jenkins/configuration/base/reconcile.go | 42 ++++++++------ .../configuration/base/reconcile_test.go | 6 +- .../configuration/base/validate_test.go | 56 +++++++++---------- pkg/controller/jenkins/groovy/groovy.go | 6 +- pkg/controller/jenkins/jenkins_controller.go | 36 +++++++----- .../jenkins/notifications/mailgun.go | 2 +- .../jenkins/notifications/msteams.go | 2 +- .../jenkins/notifications/sender.go | 15 ++--- pkg/controller/jenkins/notifications/slack.go | 12 ++-- .../jenkins/notifications/slack_test.go | 4 +- .../jenkins/plugins/base_plugins.go | 6 +- 14 files changed, 116 insertions(+), 86 deletions(-) diff --git a/go.mod b/go.mod index c2d16cd0..22aa3627 100644 --- a/go.mod +++ b/go.mod @@ -19,11 +19,11 @@ require ( github.com/stretchr/testify v1.3.0 go.uber.org/zap v1.9.1 golang.org/x/crypto v0.0.0-20190909091759-094676da4a83 // indirect - golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac // indirect + golang.org/x/lint v0.0.0-20190930215403-16217165b5de // indirect golang.org/x/net v0.0.0-20190909003024-a7b16738d86b // indirect golang.org/x/sys v0.0.0-20190910064555-bbd175535a8b // indirect golang.org/x/text v0.3.2 // indirect - golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e // indirect + golang.org/x/tools v0.0.0-20191004055002-72853e10c5a3 // indirect k8s.io/api v0.0.0-20190612125737-db0771252981 k8s.io/apimachinery v0.0.0-20190612125636-6a5db36e93ad k8s.io/client-go v11.0.0+incompatible diff --git a/go.sum b/go.sum index bc3d8a58..2d411a76 100644 --- a/go.sum +++ b/go.sum @@ -439,6 +439,8 @@ golang.org/x/lint v0.0.0-20190301231843-5614ed5bae6f h1:hX65Cu3JDlGH3uEdK7I99Ii+ golang.org/x/lint v0.0.0-20190301231843-5614ed5bae6f/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac h1:8R1esu+8QioDxo4E4mX6bFztO+dMTM49DNAaWfO5OeY= golang.org/x/lint v0.0.0-20190909230951-414d861bb4ac/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de h1:5hukYrvBGR8/eNkX5mdUezrA6JiaEZDtJb9Ei+1LlBs= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -529,6 +531,8 @@ golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578 h1:f0Gfd654rnnfXT1+BK1YHPT golang.org/x/tools v0.0.0-20190910044552-dd2b5c81c578/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e h1:1xWUkZQQ9Z9UuZgNaIR6OQOE7rUFglXUUBZlO+dGg6I= golang.org/x/tools v0.0.0-20190927191325-030b2cf1153e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20191004055002-72853e10c5a3 h1:2AmBLzhAfXj+2HCW09VCkJtHIYgHTIPcTeYqgP7Bwt0= +golang.org/x/tools v0.0.0-20191004055002-72853e10c5a3/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/api v0.0.0-20180910000450-7ca32eb868bf/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0= google.golang.org/api v0.0.0-20181030000543-1d582fd0359e/go.mod h1:4mhQ8q/RsB7i+udVvVy5NUi08OU8ZlA0gRVgrF7VFY0= diff --git a/pkg/controller/jenkins/client/script.go b/pkg/controller/jenkins/client/script.go index 61077133..ad3d1264 100644 --- a/pkg/controller/jenkins/client/script.go +++ b/pkg/controller/jenkins/client/script.go @@ -11,7 +11,12 @@ import ( ) // GroovyScriptExecutionFailed is custom error type which indicates passed groovy script is invalid -type GroovyScriptExecutionFailed struct{} +type GroovyScriptExecutionFailed struct { + ConfigurationType string + Source string + Name string + Logs string +} func (e GroovyScriptExecutionFailed) Error() string { return "script execution failed" diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 4a7cd7bf..9d1e6a7f 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -14,6 +14,7 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/backuprestore" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base/resources" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/groovy" + "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/plugins" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" @@ -39,27 +40,30 @@ const ( // ReconcileJenkinsBaseConfiguration defines values required for Jenkins base configuration type ReconcileJenkinsBaseConfiguration struct { - k8sClient client.Client - scheme *runtime.Scheme - logger logr.Logger - jenkins *v1alpha2.Jenkins - local, minikube bool - clientSet *kubernetes.Clientset - config *rest.Config + k8sClient client.Client + scheme *runtime.Scheme + logger logr.Logger + jenkins *v1alpha2.Jenkins + local, minikube bool + clientSet *kubernetes.Clientset + config *rest.Config + notificationEvents *chan notifications.Event } // New create structure which takes care of base configuration func New(client client.Client, scheme *runtime.Scheme, logger logr.Logger, - jenkins *v1alpha2.Jenkins, local, minikube bool, clientSet *kubernetes.Clientset, config *rest.Config) *ReconcileJenkinsBaseConfiguration { + jenkins *v1alpha2.Jenkins, local, minikube bool, clientSet *kubernetes.Clientset, config *rest.Config, + notificationEvents *chan notifications.Event) *ReconcileJenkinsBaseConfiguration { return &ReconcileJenkinsBaseConfiguration{ - k8sClient: client, - scheme: scheme, - logger: logger, - jenkins: jenkins, - local: local, - minikube: minikube, - clientSet: clientSet, - config: config, + k8sClient: client, + scheme: scheme, + logger: logger, + jenkins: jenkins, + local: local, + minikube: minikube, + clientSet: clientSet, + config: config, + notificationEvents: notificationEvents, } } @@ -435,6 +439,12 @@ func (r *ReconcileJenkinsBaseConfiguration) ensureJenkinsMasterPod(meta metav1.O resources.JenkinsMasterContainerName, []string{"bash", "-c", fmt.Sprintf("%s/%s && && /sbin/tini -s -- /usr/local/bin/jenkins.sh", resources.JenkinsScriptsVolumePath, resources.InitScriptName)})) } + *r.notificationEvents <- notifications.Event{ + Jenkins: *r.jenkins, + Phase: notifications.PhaseBase, + LogLevel: v1alpha2.NotificationLogLevelInfo, + Message: "Creating a new Jenkins Master Pod", + } r.logger.Info(fmt.Sprintf("Creating a new Jenkins Master Pod %s/%s", jenkinsMasterPod.Namespace, jenkinsMasterPod.Name)) err = r.createResource(jenkinsMasterPod) if err != nil { diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index 01e911f6..ea3c0afd 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -233,7 +233,7 @@ func TestCompareVolumes(t *testing.T) { Volumes: resources.GetJenkinsMasterPodBaseVolumes(jenkins), }, } - reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil) + reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil, nil) got := reconciler.compareVolumes(pod) @@ -257,7 +257,7 @@ func TestCompareVolumes(t *testing.T) { Volumes: resources.GetJenkinsMasterPodBaseVolumes(jenkins), }, } - reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil) + reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil, nil) got := reconciler.compareVolumes(pod) @@ -281,7 +281,7 @@ func TestCompareVolumes(t *testing.T) { Volumes: append(resources.GetJenkinsMasterPodBaseVolumes(jenkins), corev1.Volume{Name: "added"}), }, } - reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil) + reconciler := New(nil, nil, nil, jenkins, false, false, nil, nil, nil) got := reconciler.compareVolumes(pod) diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index ea03a478..41980de4 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -22,7 +22,7 @@ import ( func TestValidatePlugins(t *testing.T) { log.SetupLogger(true) baseReconcileLoop := New(nil, nil, log.Log, - nil, false, false, nil, nil) + nil, false, false, nil, nil, nil) t.Run("empty", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin var basePlugins []v1alpha2.Plugin @@ -163,7 +163,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateImagePullSecrets() assert.Nil(t, got) @@ -184,7 +184,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() @@ -218,7 +218,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() @@ -252,7 +252,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() @@ -286,7 +286,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() @@ -320,7 +320,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() @@ -352,7 +352,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() assert.Nil(t, got) }) @@ -378,7 +378,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() assert.Equal(t, got, []string{"Jenkins Master container env 'JENKINS_HOME' cannot be overridden"}) @@ -401,7 +401,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() assert.Equal(t, got, []string{"Jenkins Master container env 'JAVA_OPTS' doesn't have required flag '-Djava.awt.headless=true'"}) @@ -424,7 +424,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() assert.Equal(t, got, []string{"Jenkins Master container env 'JAVA_OPTS' doesn't have required flag '-Djenkins.install.runSetupWizard=false'"}) @@ -445,7 +445,7 @@ func TestValidateReservedVolumes(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateReservedVolumes() assert.Nil(t, got) }) @@ -462,7 +462,7 @@ func TestValidateReservedVolumes(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateReservedVolumes() assert.Equal(t, got, []string{"Jenkins Master pod volume 'jenkins-home' is reserved please choose different one"}) @@ -477,7 +477,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(v1alpha2.Container{}) assert.Nil(t, got) }) @@ -504,7 +504,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) assert.Nil(t, got) }) @@ -531,7 +531,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) assert.Equal(t, got, []string{"mountPath not set for 'example' volume mount in container ''"}) }) @@ -553,7 +553,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { }, } baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), - &jenkins, false, false, nil, nil) + &jenkins, false, false, nil, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) assert.Equal(t, got, []string{"Not found volume for 'missing-volume' volume mount in container ''"}) @@ -574,7 +574,7 @@ func TestValidateConfigMapVolume(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - nil, false, false, nil, nil) + nil, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateConfigMapVolume(volume) @@ -600,7 +600,7 @@ func TestValidateConfigMapVolume(t *testing.T) { err := fakeClient.Create(context.TODO(), &configMap) assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateConfigMapVolume(volume) @@ -624,7 +624,7 @@ func TestValidateConfigMapVolume(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateConfigMapVolume(volume) @@ -648,7 +648,7 @@ func TestValidateSecretVolume(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - nil, false, false, nil, nil) + nil, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateSecretVolume(volume) @@ -672,7 +672,7 @@ func TestValidateSecretVolume(t *testing.T) { err := fakeClient.Create(context.TODO(), &secret) assert.NoError(t, err) baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateSecretVolume(volume) @@ -694,7 +694,7 @@ func TestValidateSecretVolume(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateSecretVolume(volume) @@ -717,7 +717,7 @@ func TestValidateCustomization(t *testing.T) { customization := v1alpha2.Customization{} fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") @@ -737,7 +737,7 @@ func TestValidateCustomization(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) err := fakeClient.Create(context.TODO(), secret) require.NoError(t, err) @@ -766,7 +766,7 @@ func TestValidateCustomization(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) err := fakeClient.Create(context.TODO(), secret) require.NoError(t, err) err = fakeClient.Create(context.TODO(), configMap) @@ -791,7 +791,7 @@ func TestValidateCustomization(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) err := fakeClient.Create(context.TODO(), configMap) require.NoError(t, err) @@ -814,7 +814,7 @@ func TestValidateCustomization(t *testing.T) { } fakeClient := fake.NewFakeClient() baseReconcileLoop := New(fakeClient, nil, logf.ZapLogger(false), - jenkins, false, false, nil, nil) + jenkins, false, false, nil, nil, nil) err := fakeClient.Create(context.TODO(), secret) require.NoError(t, err) diff --git a/pkg/controller/jenkins/groovy/groovy.go b/pkg/controller/jenkins/groovy/groovy.go index 399ec216..dd092b04 100644 --- a/pkg/controller/jenkins/groovy/groovy.go +++ b/pkg/controller/jenkins/groovy/groovy.go @@ -49,7 +49,11 @@ func (g *Groovy) EnsureSingle(source, name, hash, groovyScript string) (requeue logs, err := g.jenkinsClient.ExecuteScript(groovyScript) if err != nil { - if _, ok := err.(*jenkinsclient.GroovyScriptExecutionFailed); ok { + if groovyErr, ok := err.(*jenkinsclient.GroovyScriptExecutionFailed); ok { + groovyErr.ConfigurationType = g.configurationType + groovyErr.Name = name + groovyErr.Source = source + groovyErr.Logs = logs g.logger.V(log.VWarn).Info(fmt.Sprintf("%s Source '%s' Name '%s' groovy script execution failed, logs :\n%s", g.configurationType, source, name, logs)) } return true, err diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 0c085fdd..ee234c22 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,7 +3,6 @@ package jenkins import ( "context" "fmt" - "reflect" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" @@ -169,7 +168,14 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul } } - if _, ok := err.(*jenkinsclient.GroovyScriptExecutionFailed); ok { + if groovyErr, ok := err.(*jenkinsclient.GroovyScriptExecutionFailed); ok { + *r.notificationEvents <- notifications.Event{ + Jenkins: *jenkins, + Phase: notifications.PhaseBase, + LogLevel: v1alpha2.NotificationLogLevelWarning, + Message: fmt.Sprintf("%s Source '%s' Name '%s' groovy script execution failed, logs:", groovyErr.ConfigurationType, groovyErr.Source, groovyErr.Name), + MessagesVerbose: []string{groovyErr.Logs}, + } return reconcile.Result{Requeue: false}, nil } return reconcile.Result{Requeue: true}, nil @@ -193,14 +199,14 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg } err = r.setDefaults(jenkins, logger) if err != nil { - return reconcile.Result{}, nil, err + return reconcile.Result{}, jenkins, err } // Reconcile base configuration - baseConfiguration := base.New(r.client, r.scheme, logger, jenkins, r.local, r.minikube, &r.clientSet, &r.config) + baseConfiguration := base.New(r.client, r.scheme, logger, jenkins, r.local, r.minikube, &r.clientSet, &r.config, r.notificationEvents) messages, err := baseConfiguration.Validate(jenkins) if err != nil { - return reconcile.Result{}, nil, err + return reconcile.Result{}, jenkins, err } if len(messages) > 0 { message := "Validation of base configuration failed, please correct Jenkins CR." @@ -215,18 +221,18 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg for _, msg := range messages { logger.V(log.VWarn).Info(msg) } - return reconcile.Result{}, nil, nil // don't requeue + return reconcile.Result{}, jenkins, nil // don't requeue } result, jenkinsClient, err := baseConfiguration.Reconcile() if err != nil { - return reconcile.Result{}, nil, err + return reconcile.Result{}, jenkins, err } if result.Requeue { - return result, nil, nil + return result, jenkins, nil } if jenkinsClient == nil { - return reconcile.Result{Requeue: false}, nil, nil + return reconcile.Result{Requeue: false}, jenkins, nil } if jenkins.Status.BaseConfigurationCompletedTime == nil { @@ -234,7 +240,7 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg jenkins.Status.BaseConfigurationCompletedTime = &now err = r.client.Update(context.TODO(), jenkins) if err != nil { - return reconcile.Result{}, nil, errors.WithStack(err) + return reconcile.Result{}, jenkins, errors.WithStack(err) } message := fmt.Sprintf("Base configuration phase is complete, took %s", @@ -253,7 +259,7 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg messages, err = userConfiguration.Validate(jenkins) if err != nil { - return reconcile.Result{}, nil, err + return reconcile.Result{}, jenkins, err } if len(messages) > 0 { message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") @@ -269,15 +275,15 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg for _, msg := range messages { logger.V(log.VWarn).Info(msg) } - return reconcile.Result{}, nil, nil // don't requeue + return reconcile.Result{}, jenkins, nil // don't requeue } result, err = userConfiguration.Reconcile() if err != nil { - return reconcile.Result{}, nil, err + return reconcile.Result{}, jenkins, err } if result.Requeue { - return result, nil, nil + return result, jenkins, nil } if jenkins.Status.UserConfigurationCompletedTime == nil { @@ -285,7 +291,7 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg jenkins.Status.UserConfigurationCompletedTime = &now err = r.client.Update(context.TODO(), jenkins) if err != nil { - return reconcile.Result{}, nil, errors.WithStack(err) + return reconcile.Result{}, jenkins, errors.WithStack(err) } message := fmt.Sprintf("User configuration phase is complete, took %s", jenkins.Status.UserConfigurationCompletedTime.Sub(jenkins.Status.ProvisionStartTime.Time)) diff --git a/pkg/controller/jenkins/notifications/mailgun.go b/pkg/controller/jenkins/notifications/mailgun.go index 45a166a2..3658abd9 100644 --- a/pkg/controller/jenkins/notifications/mailgun.go +++ b/pkg/controller/jenkins/notifications/mailgun.go @@ -65,7 +65,7 @@ func (m MailGun) Send(event Event, config v1alpha2.Notification) error { secretValue := string(secret.Data[selector.Key]) if secretValue == "" { - return errors.Errorf("Mailgun API is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) + return errors.Errorf("Mailgun API secret is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) } mg := mailgun.NewMailgun(config.Mailgun.Domain, secretValue) diff --git a/pkg/controller/jenkins/notifications/msteams.go b/pkg/controller/jenkins/notifications/msteams.go index c883fbfb..ff6811b7 100644 --- a/pkg/controller/jenkins/notifications/msteams.go +++ b/pkg/controller/jenkins/notifications/msteams.go @@ -66,7 +66,7 @@ func (t Teams) Send(event Event, config v1alpha2.Notification) error { secretValue := string(secret.Data[selector.Key]) if secretValue == "" { - return errors.Errorf("Microsoft Teams webhook URL is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) + return errors.Errorf("Microsoft Teams WebHook URL is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) } tm := &TeamsMessage{ diff --git a/pkg/controller/jenkins/notifications/sender.go b/pkg/controller/jenkins/notifications/sender.go index 441d9030..fad1219f 100644 --- a/pkg/controller/jenkins/notifications/sender.go +++ b/pkg/controller/jenkins/notifications/sender.go @@ -7,6 +7,7 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" + "github.com/pkg/errors" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -68,9 +69,9 @@ type service interface { // Listen listens for incoming events and send it as notifications func Listen(events chan Event, k8sEvent event.Recorder, k8sClient k8sclient.Client) { - for event := range events { - logger := log.Log.WithValues("cr", event.Jenkins.Name) - for _, notificationConfig := range event.Jenkins.Spec.Notifications { + for evt := range events { + logger := log.Log.WithValues("cr", evt.Jenkins.Name) + for _, notificationConfig := range evt.Jenkins.Spec.Notifications { var err error var svc service @@ -86,18 +87,18 @@ func Listen(events chan Event, k8sEvent event.Recorder, k8sClient k8sclient.Clie } go func(notificationConfig v1alpha2.Notification) { - err = notify(svc, event, notificationConfig) + err = notify(svc, evt, notificationConfig) if err != nil { if log.Debug { - logger.Error(nil, fmt.Sprintf("%+v", errors.WithMessage(err, "failed to send notification"))) + logger.Error(nil, fmt.Sprintf("%+v", errors.WithMessage(err, fmt.Sprintf("failed to send notification '%s'", notificationConfig.Name)))) } else { - logger.Error(nil, fmt.Sprintf("%s", errors.WithMessage(err, "failed to send notification"))) + logger.Error(nil, fmt.Sprintf("%s", errors.WithMessage(err, fmt.Sprintf("failed to send notification '%s'", notificationConfig.Name)))) } } }(notificationConfig) } - k8sEvent.Emit(&event.Jenkins, logLevelEventType(event.LogLevel), "NotificationSent", event.Message) + k8sEvent.Emit(&evt.Jenkins, logLevelEventType(evt.LogLevel), "NotificationSent", evt.Message) } } diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index 5ee08a98..062a61e6 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -71,18 +71,18 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { Color: s.getStatusColor(event.LogLevel), Fields: []SlackField{ { - Title: messageFieldName, + Title: "", Value: event.Message, Short: false, }, { - Title: crNameFieldName, - Value: event.Jenkins.Name, + Title: namespaceFieldName, + Value: event.Jenkins.Namespace, Short: true, }, { - Title: namespaceFieldName, - Value: event.Jenkins.Namespace, + Title: crNameFieldName, + Value: event.Jenkins.Name, Short: true, }, }, @@ -119,7 +119,7 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { secretValue := string(secret.Data[selector.Key]) if secretValue == "" { - return errors.Errorf("Secret with given name `%s` and selector name `%s` is empty", secret.Name, selector.Name) + return errors.Errorf("Slack WebHook URL is empty in secret '%s/%s[%s]", event.Jenkins.Namespace, selector.Name, selector.Key) } request, err := http.NewRequest("POST", secretValue, bytes.NewBuffer(slackMessage)) diff --git a/pkg/controller/jenkins/notifications/slack_test.go b/pkg/controller/jenkins/notifications/slack_test.go index 261a6172..b32283a6 100644 --- a/pkg/controller/jenkins/notifications/slack_test.go +++ b/pkg/controller/jenkins/notifications/slack_test.go @@ -52,14 +52,14 @@ func TestSlack_Send(t *testing.T) { assert.Equal(t, field.Value, string(event.Phase)) case crNameFieldName: assert.Equal(t, field.Value, event.Jenkins.Name) - case messageFieldName: + case "": assert.Equal(t, field.Value, event.Message) case loggingLevelFieldName: assert.Equal(t, field.Value, string(event.LogLevel)) case namespaceFieldName: assert.Equal(t, field.Value, event.Jenkins.Namespace) default: - t.Fail() + t.Errorf("Unexpected field %+v", field) } } diff --git a/pkg/controller/jenkins/plugins/base_plugins.go b/pkg/controller/jenkins/plugins/base_plugins.go index d203dd38..96b40916 100644 --- a/pkg/controller/jenkins/plugins/base_plugins.go +++ b/pkg/controller/jenkins/plugins/base_plugins.go @@ -3,12 +3,12 @@ package plugins const ( configurationAsCodePlugin = "configuration-as-code:1.29" configurationAsCodeSupportPlugin = "configuration-as-code-support:1.19" - gitPlugin = "git:3.12.0" + gitPlugin = "git:3.12.1" jobDslPlugin = "job-dsl:1.76" kubernetesCredentialsProviderPlugin = "kubernetes-credentials-provider:0.12.1" - kubernetesPlugin = "kubernetes:1.18.3" + kubernetesPlugin = "kubernetes:1.19.3" workflowAggregatorPlugin = "workflow-aggregator:2.6" - workflowJobPlugin = "workflow-job:2.34" + workflowJobPlugin = "workflow-job:2.35" ) // basePluginsList contains plugins to install by operator From 111f482e53c6d0251744708c8b6b67a86ab275d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20S=C4=99k?= Date: Sun, 6 Oct 2019 22:12:53 +0200 Subject: [PATCH 15/16] Improve notifications --- pkg/controller/jenkins/notifications/slack.go | 1 - pkg/controller/jenkins/notifications/slack_test.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/controller/jenkins/notifications/slack.go b/pkg/controller/jenkins/notifications/slack.go index 062a61e6..6468e528 100644 --- a/pkg/controller/jenkins/notifications/slack.go +++ b/pkg/controller/jenkins/notifications/slack.go @@ -86,7 +86,6 @@ func (s Slack) Send(event Event, config v1alpha2.Notification) error { Short: true, }, }, - Footer: footerContent, }, }, } diff --git a/pkg/controller/jenkins/notifications/slack_test.go b/pkg/controller/jenkins/notifications/slack_test.go index b32283a6..fa5b6c94 100644 --- a/pkg/controller/jenkins/notifications/slack_test.go +++ b/pkg/controller/jenkins/notifications/slack_test.go @@ -63,7 +63,7 @@ func TestSlack_Send(t *testing.T) { } } - assert.Equal(t, mainAttachment.Footer, footerContent) + assert.Equal(t, mainAttachment.Footer, "") assert.Equal(t, mainAttachment.Color, slack.getStatusColor(event.LogLevel)) })) From a8a380955abcfc77b8324a7b7f45b3bce0de91d1 Mon Sep 17 00:00:00 2001 From: Jakub Al-Khalili Date: Wed, 9 Oct 2019 12:26:33 +0200 Subject: [PATCH 16/16] #126 Change Configuration As Code plugin version from 1.29 to 1.32 --- pkg/controller/jenkins/plugins/base_plugins.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/jenkins/plugins/base_plugins.go b/pkg/controller/jenkins/plugins/base_plugins.go index d203dd38..1b9b9331 100644 --- a/pkg/controller/jenkins/plugins/base_plugins.go +++ b/pkg/controller/jenkins/plugins/base_plugins.go @@ -1,14 +1,14 @@ package plugins const ( - configurationAsCodePlugin = "configuration-as-code:1.29" + configurationAsCodePlugin = "configuration-as-code:1.32" configurationAsCodeSupportPlugin = "configuration-as-code-support:1.19" gitPlugin = "git:3.12.0" jobDslPlugin = "job-dsl:1.76" kubernetesCredentialsProviderPlugin = "kubernetes-credentials-provider:0.12.1" kubernetesPlugin = "kubernetes:1.18.3" workflowAggregatorPlugin = "workflow-aggregator:2.6" - workflowJobPlugin = "workflow-job:2.34" + workflowJobPlugin = "workflow-job:2.35" ) // basePluginsList contains plugins to install by operator