Fix linter golint #2240

camilamacedo86 · 2019-11-19T19:34:20Z

Closes: #2227

doc/test-framework/writing-e2e-tests.md

cmd/operator-sdk/cli/cli.go

doc/test-framework/writing-e2e-tests.md

joelanford · 2019-11-19T22:12:55Z

internal/scaffold/olm-catalog/csv_updaters.go

@@ -42,14 +42,14 @@ type CSVUpdater interface {
 	Apply(*olmapiv1alpha1.ClusterServiceVersion) error
 }

-type updaterStore struct {
+type UpdaterStore struct {


Why are we exporting this type?

The error occurs because of NewUpdaterStore which will return this structure and is used in another place. See

operator-sdk/internal/scaffold/olm-catalog/csv.go

Line 340 in 424a61d

store := NewUpdaterStore()

.

After looking at it, exporting it does make sense now. I'm okay with this.

@joelanford I understand that your only concern with this PR still this point. However, it is used outside then shows that could not be in lower case at all.

So, if you agree and it is ok for you too, I think we could move forward this one and get it merged. WDYT?

This is being removed in #2250.

Good point so, I will ignore this check for we merge this one.

H @estroz and @joelanford,

It was ignored as well. So, I think we could safely move forward here.

pkg/ansible/flags/flag.go

pkg/ansible/paramconv/paramconv.go

pkg/apis/scorecard/formatting.go

pkg/test/context.go

camilamacedo86 · 2019-11-21T15:32:33Z

/hold

jmrodri

/lgtm

jmrodri · 2019-11-25T17:02:34Z

cmd/operator-sdk/add/api.go

@@ -36,7 +36,7 @@ var (
 	skipGeneration bool
 )

-func newAddApiCmd() *cobra.Command {
+func newAddAPICmd() *cobra.Command {


jmrodri · 2019-11-25T17:10:15Z

cmd/operator-sdk/new/cmd.go

+	// The nolint here is used to hide the warning "func name will be used as new.NewCmd by other packages, and that stutters; consider calling this Cmd"
+	// which is a false positive.


can we wrap the comment so it's 80 columns per line? At least break up the first one into 2 lines.

jmrodri · 2019-11-25T17:10:42Z

cmd/operator-sdk/scorecard/cmd.go

@@ -31,7 +31,7 @@ func NewCmd() *cobra.Command {
 		Short: "Run scorecard tests",
 		Long: `Runs blackbox scorecard tests on an operator
 `,
-		RunE: scorecard.ScorecardTests,
+		RunE: scorecard.Tests,


I'm fine with this.

jmrodri · 2019-11-25T17:11:40Z

internal/flags/watch/flags.go

+	// The nolint is regards to: type name will be used as watch.WatchFlags by other packages, and that stutters; consider calling this Flags (golint)
+	// todo(camilamacedo86): Note that we decided to not introduce breakchanges to add the linters and it should be done after.
+	// From @joe: Even though watch.WatchFlags is an internal type, being embedded here means that changing it to watch.Flags is a breaking change
+	// to AnsibleOperatorFlags, which is exported. So this is a breaking change.


wrap the comments to 80 or so so I don't have to scroll to the right to read.

jmrodri · 2019-11-25T17:12:05Z

internal/scaffold/ansible/constants.go

-var AnsibleDelims = [2]string{"[[", "]]"}
+var (
+	AnsibleDelims = [2]string{"[[", "]]"}
+)


This isn't the correct fix for the lint error, which was

internal/scaffold/ansible/constants.go:30:1: comment on exported var `AnsibleDelims` should be of the form `AnsibleDelims ...` (golint)

The correct fix is:

// AnsibleDelims is a slice of two strings representing the left and right delimiters for ansible templates. Arrays can't be constants but this should be a constant. var AnsibleDelims = [2]string{"[[", "]]"}

jmrodri · 2019-11-25T17:17:39Z

internal/scaffold/olm-catalog/csv_updaters.go

@@ -42,14 +42,14 @@ type CSVUpdater interface {
 	Apply(*olmapiv1alpha1.ClusterServiceVersion) error
 }

-type updaterStore struct {
+type UpdaterStore struct {


After looking at it, exporting it does make sense now. I'm okay with this.

jmrodri · 2019-11-25T17:23:21Z

internal/scaffold/olm-catalog/csv_updaters.go

+func (store *UpdaterStore) Apply(csv *olmapiv1alpha1.ClusterServiceVersion) error {
+	updaters := []CSVUpdater{store.installStrategy, store.crds, store.almExamples}


What is the linter complaining about here? -1 I think s is just fine. I use single letters for all of my struct receivers. I agree store is easier to read, but I knew it was the store because of the receiver. It's a very common paradigm. In our broker repo there are 12260 instances of func (a *

Nevermind, I see it now. You are simply making it consistent.

jmrodri · 2019-11-25T17:30:44Z

pkg/ansible/paramconv/paramconv.go

@@ -83,10 +83,10 @@ func ToSnake(s string) string {
 	}
 	bits := []string{}
 	n := ""
-	real_i := -1
+	iReal := -1


yep I'm surprised underscore variable compiled.

jmrodri · 2019-11-25T17:32:47Z

pkg/apis/scorecard/formatting.go

+type ScorecardFormatter interface { //nolint:golint
+	// todo(camilamacedo86): The no lint here is for pkg/apis/scorecard/formatting.go:25:6: type name will be used as scorecard.ScorecardFormatter by other packages, and that stutters; consider calling this Formatter (golint)
+	// However, was decided to not move forward with it now in order to not introduce breakchanges with the task to add the linter. We should to do it after.


cmd/operator-sdk/new/cmd.go

openshift-ci-robot · 2019-11-25T17:53:09Z

New changes are detected. LGTM label has been removed.

openshift-bot · 2019-11-26T11:38:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-11-26T11:40:27Z

New changes are detected. LGTM label has been removed.

joelanford · 2019-12-02T17:50:47Z

internal/scaffold/ansible/constants.go

-var AnsibleDelims = [2]string{"[[", "]]"}
+var (
+	AnsibleDelims = [2]string{"[[", "]]"}
+)


This isn't the correct fix for the lint error, which was

internal/scaffold/ansible/constants.go:30:1: comment on exported var `AnsibleDelims` should be of the form `AnsibleDelims ...` (golint)

The correct fix is:

// AnsibleDelims is a slice of two strings representing the left and right delimiters for ansible templates. Arrays can't be constants but this should be a constant. var AnsibleDelims = [2]string{"[[", "]]"}

joelanford · 2019-12-02T17:51:20Z

internal/flags/watch/flags.go

+	/*
+		The nolint is regards to: type name will be used as watch.WatchFlags by other packages, and that stutters; consider calling this Flags (golint)
+		todo(camilamacedo86): Note that we decided to not introduce breakchanges to add the linters and it should be done after.
+		From @joe: Even though watch.WatchFlags is an internal type, being embedded here means that changing it to watch.Flags is a breaking change


Suggested change

From @joe: Even though watch.WatchFlags is an internal type, being embedded here means that changing it to watch.Flags is a breaking change

From @joelanford: Even though watch.WatchFlags is an internal type, it is embedded in exported types, which means that changing it to watch.Flags is a breaking change.

Then also delete line 31.

openshift-ci-robot · 2019-12-02T19:47:50Z

New changes are detected. LGTM label has been removed.

joelanford · 2019-12-05T17:43:35Z

internal/scaffold/olm-catalog/csv_updaters.go

+	/*
+		* todo(camilamacedo86): remove //nolint:golint after merge of https://github.com/operator-framework/operator-sdk/pull/2250
+		Skip golint because it will be changed here: https://github.com/operator-framework/operator-sdk/pull/2250
+		receiver name store should be consistent with previous receiver name s for updaterStore (golint)


This should be fixable by renaming store to s. Is there a reason we aren't doing that?

the above PR was merged. So, i will be working on it and let you know.

joelanford

One nit. Other than that, it LGTM.

joelanford · 2019-12-05T20:23:17Z

internal/scaffold/olm-catalog/csv_updaters.go

@@ -97,6 +103,7 @@ type InstallStrategyUpdate struct {
 }

 func (store *updaterStore) AddRole(yamlDoc []byte) error {
+


Nit: remove the extraneous new lines that were added in this PR.

internal/scaffold/olm-catalog/csv_updaters.go

openshift-ci-robot requested review from fabianvf and jmrodri November 19, 2019 19:34

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2019

camilamacedo86 commented Nov 19, 2019

View reviewed changes

doc/test-framework/writing-e2e-tests.md Outdated Show resolved Hide resolved

joelanford reviewed Nov 19, 2019

View reviewed changes

camilamacedo86 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 20, 2019

camilamacedo86 requested review from joelanford, asmacdo, jmccormick2001, estroz, AlexNPavel and theishshah November 20, 2019 18:50

camilamacedo86 changed the title ~~Fix linter golint~~ WIP: Fix linter golint Nov 21, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2019

camilamacedo86 changed the title ~~WIP: Fix linter golint~~ Fix linter golint Nov 21, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2019

camilamacedo86 mentioned this pull request Nov 25, 2019

Enable golint in the linter and fix issues #2227

Closed

jmrodri approved these changes Nov 25, 2019

View reviewed changes

openshift-ci-robot assigned jmrodri Nov 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2019

camilamacedo86 commented Nov 25, 2019

View reviewed changes

cmd/operator-sdk/new/cmd.go Outdated Show resolved Hide resolved

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 25, 2019

camilamacedo86 added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 26, 2019

camilamacedo86 added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 26, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

camilamacedo86 added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

joelanford requested changes Dec 2, 2019

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2019

camilamacedo86 requested a review from joelanford December 2, 2019 19:48

joelanford reviewed Dec 5, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2019

camilamacedo86 requested a review from joelanford December 5, 2019 20:17

joelanford approved these changes Dec 5, 2019

View reviewed changes

camilamacedo86 commented Dec 5, 2019

View reviewed changes

internal/scaffold/olm-catalog/csv_updaters.go Outdated Show resolved Hide resolved