Merge pull request #1818 from jarrpa/merge-jarrpa-dev

Various fixes to initial GitHub Actions

.github/workflows/ocs-operator-ci.yaml

@@ -19,37 +19,41 @@ jobs:
           fetch-depth: 0
 
       - name: Run shellcheck
-        run: make shellcheck
+        run: make shellcheck-test
 
   golangci-lint:
     name: golangci-lint
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
       matrix:
-        go: ["1.17", "1.18"]
+        go: ["1.18","1.19"]
     steps:
-      - uses: actions/setup-go@v3
-        with:
-          go-version: ${{ matrix.go }}
-          go-version-file: go.mod
-
       - uses: actions/checkout@v3
         with:
           fetch-depth: 0
 
+      # TODO: Our code currently does not pass linting using Go 1.19, so
+      # avoiding it until we bump go.mod to 1.19 as well.
+      - uses: actions/setup-go@v3
+        with:
+          go-version-file: go.mod
+
       - uses: golangci/golangci-lint-action@v3
         with:
           version: v1.49.0
-          args: -E gosec --timeout=6m
+
+          # The weird NO_FUTURE thing is a workaround suggested here:
+          # # https://github.com/golangci/golangci-lint-action/issues/119#issuecomment-981090648
+          args: "--out-${NO_FUTURE}format=colored-line-number --timeout=6m ./..."
 
   go-test:
-    name: go-test unit tests
+    name: go test
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
       matrix:
-        go: ["1.17", "1.18"]
+        go: ["1.18", "1.19"]
     steps:
       - uses: actions/setup-go@v3
         with:
@@ -69,8 +73,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ["1.17", "1.18"]
-        make-target: ["verify-deps", "verify-generated", "verify-latest-csv", "verify-operator-bundle", "verify-latest-deploy-yaml"]
+        go: ["1.18", "1.19"]
     steps:
       - uses: actions/setup-go@v3
         with:
@@ -80,5 +83,17 @@ jobs:
         with:
           fetch-depth: 0
 
-      - name: Run ${{ matrix.make-target }} make target
-        run: make ${{ matrix.make-target }}
+      - name: Verify go dependencies
+        run: make verify-deps
+
+      - name: Verify generated code and configd
+        run: make verify-generated
+
+      - name: Verify CSV changes
+        run: make verify-latest-csv
+
+      - name: Verify bundle changes
+        run: make verify-operator-bundle
+
+      - name: Verify deployment YAML
+        run: make verify-latest-deploy-yaml

.golangci.yaml

@@ -9,14 +9,11 @@ run:
 linters:
   disable-all: true
   enable:
-    - deadcode
     - errcheck
     - gosimple
     - govet
     - ineffassign
     - staticcheck
-    - structcheck
     - unused
-    - varcheck
     - gofmt
     - revive

Makefile

@@ -113,7 +113,7 @@ verify-latest-csv: gen-latest-csv
 	@echo "Verifying latest CSV"
 	hack/verify-latest-csv.sh
 
-verify-operator-bundle:
+verify-operator-bundle: operator-sdk
 	@echo "Verifying operator bundle"
 	hack/verify-operator-bundle.sh
 

api/v1alpha1/groupversion_info.go

@@ -15,8 +15,8 @@ limitations under the License.
 */
 
 // Package v1alpha1 contains API Schema definitions for the ocs v1alpha1 API group
-//+kubebuilder:object:generate=true
-//+groupName=ocs.openshift.io
+// +kubebuilder:object:generate=true
+// +groupName=ocs.openshift.io
 package v1alpha1
 
 import (

controllers/ocsinitialization/ocsinitialization_controller.go

@@ -43,7 +43,7 @@ func InitNamespacedName() types.NamespacedName {
 }
 
 // OCSInitializationReconciler reconciles a OCSInitialization object
-//nolint
+// nolint:revive
 type OCSInitializationReconciler struct {
 	client.Client
 	Log            logr.Logger

controllers/ocsinitialization/ocsinitialization_controller_test.go

@@ -140,7 +140,6 @@ func TestNonWatchedResourceNotFound(t *testing.T) {
 	}
 }
 
-//nolint //ignoring errcheck causing the failures
 func TestNonWatchedResourceFound(t *testing.T) {
 	testcases := []struct {
 		label     string
@@ -160,6 +159,7 @@ func TestNonWatchedResourceFound(t *testing.T) {
 	}
 
 	for _, tc := range testcases {
+		ctx := context.TODO()
 		_, _, reconciler := getTestParams(true, t)
 		request := reconcile.Request{
 			NamespacedName: types.NamespacedName{
@@ -173,18 +173,17 @@ func TestNonWatchedResourceFound(t *testing.T) {
 				Namespace: tc.namespace,
 			},
 		}
-		err := reconciler.Client.Create(nil, &ocs)
+		err := reconciler.Client.Create(ctx, &ocs)
 		assert.NoErrorf(t, err, "[%s]: failed CREATE of non watched resource", tc.label)
 		_, err = reconciler.Reconcile(context.TODO(), request)
 		assert.NoErrorf(t, err, "[%s]: failed to reconcile with non watched resource", tc.label)
 		actual := &v1.OCSInitialization{}
-		err = reconciler.Client.Get(nil, request.NamespacedName, actual)
+		err = reconciler.Client.Get(ctx, request.NamespacedName, actual)
 		assert.NoErrorf(t, err, "[%s]: failed GET of actual resource", tc.label)
 		assert.Equalf(t, statusutil.PhaseIgnored, actual.Status.Phase, "[%s]: failed to update phase of non watched resource that already exists OCS:\n%v", tc.label, actual)
 	}
 }
 
-//nolint //ignoring errcheck as causing failures
 func TestCreateWatchedResource(t *testing.T) {
 	testcases := []struct {
 		label          string
@@ -201,20 +200,19 @@ func TestCreateWatchedResource(t *testing.T) {
 	}
 
 	for _, tc := range testcases {
+		ctx := context.TODO()
 		ocs, request, reconciler := getTestParams(false, t)
-		if tc.alreadyCreated {
-			reconciler.Client.Create(nil, &ocs)
-		} else {
-			err := reconciler.Client.Delete(nil, &ocs)
+		if !tc.alreadyCreated {
+			err := reconciler.Client.Delete(ctx, &ocs)
 			assert.NoError(t, err)
 
-			err = reconciler.Client.Get(nil, request.NamespacedName, &ocs)
+			err = reconciler.Client.Get(ctx, request.NamespacedName, &ocs)
 			assert.Error(t, err)
 		}
-		_, err := reconciler.Reconcile(context.TODO(), request)
+		_, err := reconciler.Reconcile(ctx, request)
 		assert.NoError(t, err)
 		obj := v1.OCSInitialization{}
-		_ = reconciler.Client.Get(nil, request.NamespacedName, &obj)
+		_ = reconciler.Client.Get(ctx, request.NamespacedName, &obj)
 		assert.Equalf(t, obj.Name, request.Name, "[%s]: failed to create ocsInit resource with correct name", tc.label)
 		assert.Equalf(t, obj.Namespace, request.Namespace, "[%s]: failed to create ocsInit resource with correct namespace", tc.label)
 	}

controllers/storageclassclaim/storageclassclaim_controller.go

@@ -58,7 +58,7 @@ const (
 )
 
 // StorageClassClaimReconciler reconciles a StorageClassClaim object
-// nolint
+// nolint:revive
 type StorageClassClaimReconciler struct {
 	client.Client
 	cache.Cache

controllers/storagecluster/external_resources_test.go

@@ -415,7 +415,6 @@ func TestExternalResourceReconcile(t *testing.T) {
 	assertReconciliationOfExternalResource(t, reconciler)
 }
 
-// nolint
 func assertReconciliationOfExternalResource(t *testing.T, reconciler StorageClusterReconciler) {
 	request := reconcile.Request{
 		NamespacedName: types.NamespacedName{
@@ -424,14 +423,16 @@ func assertReconciliationOfExternalResource(t *testing.T, reconciler StorageClus
 		},
 	}
 
+	ctx := context.TODO()
+
 	// first reconcile, which sets everything in place
-	result, err := reconciler.Reconcile(context.TODO(), request)
+	result, err := reconciler.Reconcile(ctx, request)
 	assert.NoError(t, err)
 	assert.Equal(t, reconcile.Result{}, result)
 	assertExpectedExternalResources(t, reconciler)
 
 	sc := &api.StorageCluster{}
-	err = reconciler.Client.Get(nil, request.NamespacedName, sc)
+	err = reconciler.Client.Get(ctx, request.NamespacedName, sc)
 	assert.NoError(t, err)
 	firstExtSecretChecksum := sc.Status.ExternalSecretHash
 
@@ -448,35 +449,35 @@ func assertReconciliationOfExternalResource(t *testing.T, reconciler StorageClus
 	extSecret, err := createExternalCephClusterSecret(extRsrcs)
 	assert.NoError(t, err)
 	secret := corev1.Secret{}
-	reconciler.Client.Get(nil, types.NamespacedName{Name: externalClusterDetailsSecret, Namespace: ""}, &secret)
+	err = reconciler.Client.Get(ctx, types.NamespacedName{Name: externalClusterDetailsSecret, Namespace: ""}, &secret)
 	assert.NoError(t, err)
 	extSecret.ObjectMeta = secret.ObjectMeta
-	err = reconciler.Client.Update(nil, extSecret)
+	err = reconciler.Client.Update(ctx, extSecret)
 	assert.NoError(t, err)
 
 	// second reconcile on same 'reconciler', we should have expected/changed resources
-	result, err = reconciler.Reconcile(context.TODO(), request)
+	result, err = reconciler.Reconcile(ctx, request)
 	assert.NoError(t, err)
 	assert.Equal(t, reconcile.Result{}, result)
 	assertExpectedExternalResources(t, reconciler)
 
 	// get the updated storagecluster object after second reconciliation
 	sc = &api.StorageCluster{}
-	err = reconciler.Client.Get(nil, request.NamespacedName, sc)
+	err = reconciler.Client.Get(ctx, request.NamespacedName, sc)
 	assert.NoError(t, err)
 	secondExtSecretChecksum := sc.Status.ExternalSecretHash
 	// as there are changes, first and second checksums should not match
 	assert.NotEqual(t, firstExtSecretChecksum, secondExtSecretChecksum)
 
 	// third reconcile on same 'reconciler', without any change in the resources
-	result, err = reconciler.Reconcile(context.TODO(), request)
+	result, err = reconciler.Reconcile(ctx, request)
 	assert.NoError(t, err)
 	assert.Equal(t, reconcile.Result{}, result)
 	assertExpectedExternalResources(t, reconciler)
 
 	// get the updated storagecluster object after third reconciliation
 	sc = &api.StorageCluster{}
-	err = reconciler.Client.Get(nil, request.NamespacedName, sc)
+	err = reconciler.Client.Get(ctx, request.NamespacedName, sc)
 	assert.NoError(t, err)
 	thirdExtSecretChecksum := sc.Status.ExternalSecretHash
 	// as there are no changes, second and third checksums should match

controllers/storagecluster/placement.go

@@ -79,7 +79,7 @@ func getPlacement(sc *ocsv1.StorageCluster, component string) rookCephv1.Placeme
 	return placement
 }
 
-//convertLabelToNodeSelectorRequirements returns a NodeSelectorRequirement list from a given LabelSelector
+// convertLabelToNodeSelectorRequirements returns a NodeSelectorRequirement list from a given LabelSelector
 func convertLabelToNodeSelectorRequirements(labelSelector metav1.LabelSelector) []corev1.NodeSelectorRequirement {
 	reqs := []corev1.NodeSelectorRequirement{}
 	for key, value := range labelSelector.MatchLabels {
@@ -122,7 +122,7 @@ type MatchingLabelsSelector struct {
 }
 
 // ApplyToList applies this configuration to the given list options.
-//This is implemented by MatchingLabelsSelector which implements ListOption interface.
+// This is implemented by MatchingLabelsSelector which implements ListOption interface.
 func (m MatchingLabelsSelector) ApplyToList(opts *client.ListOptions) {
 	opts.LabelSelector = m
 }

controllers/storagecluster/provider_server.go

@@ -344,7 +344,7 @@ func GetProviderAPIServerSecret(instance *ocsv1.StorageCluster) *corev1.Secret {
 	}
 }
 
-//RandomString - Generate a random string of A-Z chars with len = l
+// RandomString - Generate a random string of A-Z chars with len = l
 func RandomString(l int) string {
 
 	bytes := make([]byte, l)

controllers/storagecluster/storagecluster_controller.go

@@ -74,7 +74,7 @@ type ImageMap struct {
 }
 
 // StorageClusterReconciler reconciles a StorageCluster object
-//nolint
+// nolint:revive
 type StorageClusterReconciler struct {
 	client.Client
 	ctx                context.Context

controllers/storagecluster/storagecluster_controller_test.go

@@ -1046,7 +1046,6 @@ func createFakeScheme(t *testing.T) *runtime.Scheme {
 	return scheme
 }
 
-//nolint //ignoring err checks as causing failures
 func TestMonCountChange(t *testing.T) {
 	for nodeCount := 0; nodeCount <= 10; nodeCount++ {
 		monCountExpected := defaults.DefaultMonCount

controllers/storagecluster/uninstall_reconciler.go

@@ -41,8 +41,10 @@ const (
 	UninstallModeGraceful UninstallModeType = "graceful"
 )
 
-//nolint:unused // func deleteNodeAffinityKeyFromNodes is not used. For Future usuage func is created.
 // deleteNodeAffinityKeyFromNodes deletes the default NodeAffinityKey from the OCS nodes
+// This is not used, yet.
+//
+// nolint:unused
 func (r *StorageClusterReconciler) deleteNodeAffinityKeyFromNodes(sc *ocsv1.StorageCluster) (err error) {
 
 	// We should delete the label only when the StorageCluster is using the default NodeAffinityKey

controllers/storagecluster/uninstall_reconciler_test.go

@@ -205,7 +205,8 @@ func TestDeleteSnapshotClasses(t *testing.T) {
 	}
 }
 
-//nolint // func assertTestDeleteSnapshotClasses is not used. For Future usuage func is created.
+// This function is not used, yet. It will be used in the future.
+// nolint:unused
 func assertTestDeleteSnapshotClasses(
 	t *testing.T, reconciler StorageClusterReconciler, sc *api.StorageCluster, SnapshotClassExists bool) {
 
@@ -545,7 +546,8 @@ func TestDeleteCephBlockPools(t *testing.T) {
 	}
 }
 
-//nolint // func assertTestDeleteCephBlockPools is not used. For Future usuage func is created.
+// This function is not used, yet. It will be used in the future.
+// nolint:unused
 func assertTestDeleteCephBlockPools(
 	t *testing.T, reconciler StorageClusterReconciler, sc *api.StorageCluster, cephBlockPoolsExist bool) {
 

functests/common.go

@@ -20,7 +20,7 @@ func debug(msg string, args ...interface{}) {
 	ginkgo.GinkgoWriter.Write([]byte(fmt.Sprintf(msg, args...)))
 }
 
-//RunMustGather runs the OCS must-gather container image
+// RunMustGather runs the OCS must-gather container image
 func RunMustGather() error {
 	gopath := os.Getenv("GOPATH")
 	cmd := exec.Command("/bin/bash", gopath+"/src/github.com/red-hat-storage/ocs-operator/hack/dump-debug-info.sh")

hack/golangci_lint.sh

@@ -6,7 +6,7 @@ source hack/common.sh
 
 mkdir -p ${OUTDIR_TOOLS}
 LINT_BIN="${OUTDIR_TOOLS}/golangci-lint"
-LINT_VER="1.47.3"
+LINT_VER="1.49.0"
 
 check_bin_exists() {
   which "${LINT_BIN}" >/dev/null 2>&1 && [[ "$(${LINT_BIN} --version)" == *"${LINT_VER}"* ]]

hack/verify-operator-bundle.sh

@@ -2,8 +2,7 @@
 
 set -e
 
-source hack/common.sh
+source hack/ensure-operator-sdk.sh
 source hack/docker-common.sh
-source hack/operator-sdk-common.sh
 
-./"${OPERATOR_SDK}" bundle validate "$(dirname $OCS_FINAL_DIR)" -b "$IMAGE_BUILD_CMD" --verbose
+./"${OPERATOR_SDK}" bundle validate "$(dirname $OCS_FINAL_DIR)" --verbose

metrics/internal/collectors/object-bucket_test.go

@@ -22,7 +22,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-/*const (
+/*
+const (
 getUserJSON = )
 */
 var (

metrics/internal/exporter/registry.go

@@ -10,6 +10,7 @@ import (
 //   - prometheus.NewProcessCollector
 //   - prometheus.NewGoCollector
 //   - collectors.NewExporterVersionCollector
+//
 // This is intended to be used to expose metrics about the exporter.
 func RegisterExporterCollectors(registry *prometheus.Registry) {
 	registry.MustRegister(