Adds linting CI workflow #264

m1kola · 2023-06-15T14:51:45Z

Description

This PR adds linting CI workflow which is based on the one from rukpak

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

m1kola

There are a lot of small changes to address linting issues. Hopefully these comments will help with reviews.

m1kola · 2023-06-16T12:34:42Z

internal/controllers/operator_controller.go

+//
+// Today we always return ctrl.Result{} and an error.
+// But in the future we might update this function
+// to return different results (e.g. requeue).
+//
+//nolint:unparam


Same as in rukpak (see this thread).

m1kola · 2023-06-16T12:35:18Z

internal/resolution/entitysources/catalogdsource.go

 // made accessible on-cluster by https://github.com/operator-framework/catalogd.
 // It is an implementation of deppy defined input.EntitySource
-type catalogdEntitySource struct {
+type CatalogdEntitySource struct {


NewCatalogdEntitySource was returning unexported struct.

m1kola · 2023-06-16T12:39:00Z

internal/resolution/variable_sources/crdconstraints/crd_constraints.go

@@ -1,4 +1,4 @@
-package crd_constraints


I'm a bit on the fence with this one. Lookign for opinions. I can revert and we can disable this check.

While valid and compiles this considered to not be idiomatic in Go.

Same goes for bundles_and_dependencies and required_package.

See:

https://go.dev/doc/effective_go#package-names

https://go.dev/blog/package-names

I'm +1 on these changes. I don't know of any packages in any dependencies I've used that have underscores in their paths or names.

This may be an indication that we don't have the right package organization in the first place.

I would say it's fine to make the change as-is for now, but I would personally like to see a bit of a change in the directory structure to make it a bit easier to make changes and add new features. For example, in the current structure to add a new k8s version constraint we would probably end up creating a new package/directory called k8sversionconstraint.

Instead, I think we should follow a pattern more like constraints/crd. Following the above example, it is intuitive where a new constraint package should be added and would be constraints/kubeversion (or something like that). IMO the same should go for bundles_and_dependencies (what do they represent? can they be broken into separate packages?) and required_packages (how should we add new required_*?)

Or even just package constraints with types CRD and KubernetesVersion. Or maybe even more combined, package olm with CRDConstraint, KubernetesVersionConstraint, BundleVariable, etc.

Do we need all of the package sprawl?

@joelanford @everettraven I'm in in favour of starting with just one constraints package and splitting it into more once we see the need and/or once/if we decide to make this stuff exportable.

But I think it should be done outside of this PR (which is already fairly big).

But I think it should be done outside of this PR (which is already fairly big).

Yeah, definitely +1

m1kola · 2023-06-16T12:41:04Z

internal/controllers/suite_test.go

 	items, err := meta.ExtractList(list)
 	Expect(err).NotTo(HaveOccurred())
+
+	names := make([]string, 0, len(items))


Preallocation of the slice. There are few other places like this

https://golangci-lint.run/usage/linters/#prealloc

m1kola · 2023-06-16T12:44:44Z

internal/resolution/variable_sources/requiredpackage/required_package.go

-type RequiredPackageVariable struct {
+type Variable struct {


Linter complained about name stuttering: when exported requiredpackage.RequiredPackageVariable vs requiredpackage.Variable.

Same for RequiredPackageOption vs Option and RequiredPackageVariableSource vs VariableSource

m1kola · 2023-06-16T13:55:03Z

internal/resolution/entitysources/catalogdsource.go

-		if filter(&entity) {
-			resultSet = append(resultSet, entity)
+	for i := range entities {
+		if filter(&entities[i]) {


gosec: G601: Implicit memory aliasing in for loop.

+1 on this fix. However it makes me wonder... is it necessary or important that input.Predicate takes a pointer to an input.Entity? Could we take a input.Entity rather than an *input.Entity?

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola · 2023-06-20T13:40:42Z

Rebased on top of master to fix conflicts with #263 which just merged.

m1kola · 2023-06-20T13:42:47Z

pkg/features/features.go

@@ -17,5 +18,5 @@ var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.Feature
 var OperatorControllerFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()

 func init() {
-	OperatorControllerFeatureGate.Add(operatorControllerFeatureGates)
+	utilruntime.Must(OperatorControllerFeatureGate.Add(operatorControllerFeatureGates))


@everettraven is it fair? Or do we need better error handling here?

I think that is fair. Good catch - I totally missed that it returned an error. Would probably be good to fix this in catalogd as well (and add linting there :))

@everettraven here is the PR for catalogd - operator-framework/catalogd#98

everettraven · 2023-06-20T13:34:23Z

internal/controllers/operator_controller.go

-// verifyBDStatus reads the various possibilities of status in bundle deployment and translates
-// into corresponding operator condition status and message.
-func verifyBDStatus(dep *rukpakv1alpha1.BundleDeployment) (metav1.ConditionStatus, string) {
-	isValidBundleCond := apimeta.FindStatusCondition(dep.Status.Conditions, rukpakv1alpha1.TypeHasValidBundle)
-	isInstalledCond := apimeta.FindStatusCondition(dep.Status.Conditions, rukpakv1alpha1.TypeInstalled)
-
-	if isValidBundleCond != nil && isValidBundleCond.Status == metav1.ConditionFalse {
-		return metav1.ConditionFalse, isValidBundleCond.Message
-	}
-
-	if isInstalledCond != nil && isInstalledCond.Status == metav1.ConditionFalse {
-		return metav1.ConditionFalse, isInstalledCond.Message
-	}
-
-	if isInstalledCond != nil && isInstalledCond.Status == metav1.ConditionTrue {
-		return metav1.ConditionTrue, "install was successful"
-	}
-	return metav1.ConditionUnknown, fmt.Sprintf("could not determine the state of BundleDeployment %s", dep.Name)
-}
-
-// isBundleDepStale returns true if conditions are out of date.
-func isBundleDepStale(bd *rukpakv1alpha1.BundleDeployment) bool {
-	return bd != nil && bd.Status.ObservedGeneration != bd.GetGeneration()
-}
-


Was all this unused code?

everettraven · 2023-06-20T13:46:36Z

internal/resolution/variable_sources/crdconstraints/crd_constraints.go

@@ -1,4 +1,4 @@
-package crd_constraints


I would say it's fine to make the change as-is for now, but I would personally like to see a bit of a change in the directory structure to make it a bit easier to make changes and add new features. For example, in the current structure to add a new k8s version constraint we would probably end up creating a new package/directory called k8sversionconstraint.

Instead, I think we should follow a pattern more like constraints/crd. Following the above example, it is intuitive where a new constraint package should be added and would be constraints/kubeversion (or something like that). IMO the same should go for bundles_and_dependencies (what do they represent? can they be broken into separate packages?) and required_packages (how should we add new required_*?)

ankitathomas

This looks really good, I have a few comments but nothing blocking

ankitathomas · 2023-06-20T15:53:05Z

internal/controllers/operator_controller_test.go

-	err := c.Get(ctx, key, op)
-	Expect(err).To(BeNil())
+	Expect(c.Get(ctx, key, op)).To(Succeed())


I see a few other places where we're using Expect(err).NotTo(HaveOccurred()). Are we looking to standardize this throughout the test suite?

As far as I understand Expect(funcCall()).To(Succeed()) and Expect(err).NotTo(HaveOccurred()) do the same thing, but the first one is supposed to be used with functions which only return one error. I personally not too concerned about using one style over another. Important one is to use one of these styles overExpect(err).To(BeNil()) because they give better failure messages.

In general, I'm not a big fan of Ginkgo & Gomega because there are usually 10 ways to do the same.

ankitathomas · 2023-06-20T16:07:30Z

internal/resolution/variable_sources/bundlesanddependencies/bundles_and_dependencies_test.go

 				bundleVariables = append(bundleVariables, v)
 			}
 		}
-		Expect(len(bundleVariables)).To(Equal(12))
+		Expect(bundleVariables).To(HaveLen(12))
 		Expect(bundleVariables).To(WithTransform(CollectBundleVariableIDs, Equal([]string{"bundle-2", "bundle-1", "bundle-15", "bundle-16", "bundle-17", "bundle-9", "bundle-8", "bundle-7", "bundle-5", "bundle-4", "bundle-11", "bundle-10"})))


nit: do we need to have the length check if we're already checking equality?

Good point. I wasn't paying too much attention to the surrounding lines only validated that ginkgolinter --fix ./... gave me reasonable fixes.

I'll submit a follow up request to delete this line & update to use ConsistOf as you suggested in another comment.

ankitathomas · 2023-06-20T16:21:52Z

internal/resolution/variable_sources/crdconstraints/crd_constraints_test.go

 				crdConstraintVariables = append(crdConstraintVariables, v)
 			}
 		}
-		Expect(len(crdConstraintVariables)).To(Equal(11))
+		Expect(crdConstraintVariables).To(HaveLen(11))
 		Expect(crdConstraintVariables).To(WithTransform(CollectGlobalConstraintVariableIDs, ContainElements([]string{


ConsistOf should help avoid the length check here

ankitathomas · 2023-06-20T16:37:16Z

internal/resolution/variable_sources/bundlesanddependencies/bundles_and_dependencies.go

@@ -1,4 +1,4 @@
-package bundles_and_dependencies


nit: can we rename this package at all? bundlesanddependencies seems unnecessarily long, bundleentity or bundlesource might do the job just as well

There is a relevant discussion on this here: #264 (comment)

I'll look into better structure and names in a separate PR as this one is quite large already.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2023

m1kola force-pushed the add_linters branch 2 times, most recently from e3d5805 to 33e4b83 Compare June 16, 2023 08:46

m1kola commented Jun 16, 2023

View reviewed changes

m1kola marked this pull request as ready for review June 16, 2023 12:51

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2023

m1kola commented Jun 16, 2023

View reviewed changes

m1kola added 4 commits June 20, 2023 14:36

Adds lint, tidy and verify make targets

8cfbd9c

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Fixes linting issues

91c586e

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Renames package with underscores

c41b117

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Removes name stuttering

73646a5

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola force-pushed the add_linters branch from 33e4b83 to 73646a5 Compare June 20, 2023 13:40

m1kola commented Jun 20, 2023

View reviewed changes

everettraven reviewed Jun 20, 2023

View reviewed changes

joelanford mentioned this pull request Jun 20, 2023

Add a GitHub Action for linting operator-framework/catalogd#97

Closed

ankitathomas approved these changes Jun 20, 2023

View reviewed changes

m1kola merged commit a946081 into operator-framework:main Jun 22, 2023

m1kola deleted the add_linters branch June 22, 2023 09:14

This was referenced Jun 22, 2023

Removes unnecessary HaveLen assertions #272

Merged

Changes the structure of resolution packages #273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds linting CI workflow #264

Adds linting CI workflow #264

m1kola commented Jun 15, 2023

m1kola left a comment

m1kola Jun 16, 2023

m1kola Jun 16, 2023

m1kola Jun 16, 2023

joelanford Jun 20, 2023

everettraven Jun 20, 2023

joelanford Jun 20, 2023

m1kola Jun 20, 2023

joelanford Jun 20, 2023

m1kola Jun 16, 2023

m1kola Jun 16, 2023 •

edited

Loading

m1kola Jun 16, 2023

joelanford Jun 20, 2023

m1kola commented Jun 20, 2023

m1kola Jun 20, 2023

everettraven Jun 20, 2023

m1kola Jun 20, 2023

everettraven Jun 20, 2023

m1kola Jun 20, 2023

everettraven Jun 20, 2023

ankitathomas left a comment

ankitathomas Jun 20, 2023

m1kola Jun 22, 2023

ankitathomas Jun 20, 2023

m1kola Jun 22, 2023

ankitathomas Jun 20, 2023

ankitathomas Jun 20, 2023

m1kola Jun 22, 2023

Adds linting CI workflow #264

Adds linting CI workflow #264

Conversation

m1kola commented Jun 15, 2023

Description

Reviewer Checklist

m1kola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitathomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola Jun 16, 2023 •

edited

Loading