run-checks: added golangci-lint to run-checks #14464

maykathm · 2024-09-03T15:30:56Z

This adds the use of golangci-lint snap both in run-checks.sh and in the CI. The change in the CI's serves to align a local run-checks.sh run with a CI run. By using the same snap and ensuring it's always up to date in both cases, developers should know what the CI is going to tell them before triggering a runner.

run-checks

olivercalder

Looks good, thanks for working on this! I have a few thoughts/questions about versions of golangci-lint/go which are being used. The only blocker for me is I think we should record an error if the user is not tracking latest/stable for golangci-lint in addition to if there's a pending update. And I think maybe we should continue to run golangci-lint on go 1.18/stable as well as latest/stable.

.github/workflows/test.yaml

run-checks

olivercalder

Some more thoughts on synchronization between golangci-lint version and go version.

.github/workflows/test.yaml

olivercalder

Maybe we should leave our current CI as-is, with the exception of using the golangci-lint snap instead of the github workflow version.

I'm also a bit wary to turn off golangci-lint for 1.18/stable (and leaving it on for latest/stable) in CI. Afaict new golangci-lint should remain compatible with old go versions.

.github/workflows/test.yaml

olivercalder

Looks really good, thank you! A few final comments. Whether to run golangci-lint on non-latest/stable systems is what I'm most concerned about.

olivercalder · 2024-09-06T17:50:30Z

run-checks

+        goci_go_ver=$(golangci-lint --version | grep -o 'go[0-9]*\.[0-9]*\.[0-9]*' | cut -c 3-)
+        go_ver=$(go version | grep -o 'go[0-9]*\.[0-9]*\.[0-9]*' | cut -c 3-)
+        if [ "$(printf '%s\n' "$go_ver" "$goci_go_ver" | sort -V | head -n1)" != "$go_ver" ]; then
+           echo "WARNING: Your go version ($go_ver) is greater than the version of go that golangci-lang was built with ($goci_go_ver)."


Suggested change

echo "WARNING: Your go version ($go_ver) is greater than the version of go that golangci-lang was built with ($goci_go_ver)."

echo "WARNING: Your go version ($go_ver) is greater than the version of go that golangci-lint was built with ($goci_go_ver)."

olivercalder · 2024-09-06T17:53:47Z

.github/workflows/test.yaml

+          if [ "${{ matrix.gochannel }}" != "latest/stable" ]; then
+              export SKIP_GOLANG_CI_LINT=1
+              echo "Will skip golangci_lint checks on all but the latest/stable version"


Sorry to return to this, but I think maybe we should run golangci-lint regardless of go versions. New golangci-lint should work with old go versions, so that's not a problem.

The big reason for this though is that if there is a linting error, it will appear as though 1.18/stable static checks passed and latest/stable static checks failed, implying that there's a problem with something related to the new version of go, when in reality, it may just be a linting problem caught by golangci-lint, which happened to only be run on latest/stable.

So I'd rather remove this check/skip here.

Oops you're right! I forgot to remove that skip after your last comments. Sorry about that.

olivercalder · 2024-09-06T17:58:58Z

tests/unit/go/task.yaml

+    # To avoid checking old code, skip golangci-lint
+    skip="${skip:-} SKIP_GOLANG_CI_LINT=1"
+


If the concern is about checking old code, could we inject a variable with BASE_REF into these spread tests?

If it's just about not redoing the lint checks on every system, I'd rather adjust the comment to say as much.

FWIW, I think it's probably fine to skip golangci-lint checks in spread, as this just checks source code and should be system-agnostic. And the spread systems probably don't have the golangci-lint installed anyway.

olivercalder · 2024-09-06T18:01:18Z

.github/workflows/test.yaml

@@ -223,6 +212,10 @@ jobs:
              export SKIP_GOFMT=1
              echo "Formatting checks will be skipped due to the use of Go version ${{ matrix.gochannel }}"
          fi
+          if [ "${{ matrix.gochannel }}" != "latest/stable" ]; then
+              export SKIP_GOLANG_CI_LINT=1


Super nitpicky nitpick, feel free to ignore: I think SKIP_GOLANGCI_LINT would be more consistent than SKIP_GOLANG_CI_LINT, it's already hard enough to remember where the hyphen goes in the binary name :)

…nd made clearer env variable and comment

bboozzoo · 2024-09-10T10:31:23Z

run-checks

+        if snap refresh --list | grep -q golangci-lint; then
+            echo "WARNING: your golangci-lint snap is out of date. The CI will install a fresh version, which may differ from yours."
+        fi
+        if ! snap list golangci-lint | grep -q latest; then
+            echo "WARNING: your golangci-lint snap is not installed from the latest/* channel."
+        fi


I suppose those are only relevant iff using golangci-lint from a snap which isn't always the case. Perhaps :

gcil="$(command -v golangci-lint || true)" if [ -z "$gcil" ]; then echo "ERROR: Cannot find golangci-lint. You need to first install the golangci-lint" exit 1 fi if [ "${gcil/\/snap\/bin/}" != "$gcil" ]; then # installed from snap # check refresh # check latest fi # check built-with-go-version etc.

Good point. I added your changes. Thanks!

…olangci-lint

olivercalder

Thanks for all your work on this, looks really good! Two nitpicks, but neither of them are blockers.

olivercalder · 2024-09-10T23:05:41Z

run-checks

-        if ! snap list golangci-lint | grep -q latest; then
-            echo "WARNING: your golangci-lint snap is not installed from the latest/* channel."
+
+        if [ "${gcil/\/snap\/bin/}" != "$gcil" ]; then


Nitpick: I'm not familiar with this construction, is this a bash-ism for running a replacement on a variable? If this is bash-specific, I'd stick to POSIX shell and do something a bit more explicit. If this is supported by POSIX-compliant shells then (oh god) it's fine, but I'd still personally prefer something easier to read.

Edit: looks like zsh supports this, but dash doesn't seem to like it:

oac@xps15  ~  foo="hello" oac@xps15  ~  echo $foo hello oac@xps15  ~  echo "${foo/ll/}" heo oac@xps15  ~  echo "${foo/ll/aa/}" heaa/o oac@xps15  ~  echo "${foo/ll/aa}" heaao oac@xps15  ~  dash $ foo="hello" $ echo $foo hello $ echo "${foo/ll/aa}" dash: 3: Bad substitution $ echo "${foo/ll/}" dash: 4: Bad substitution

Not sure what the takeaway is from this... I think I'd suggest to just do this instead:

Suggested change

if [ "${gcil/\/snap\/bin/}" != "$gcil" ]; then

if echo "$gcil" | grep '^\/snap\/bin\/' ; then

it's likely that ${var/<pattern>/<subst>} works everywhere but dash, alternative would be to use:

"${gcil%/snap/bin/golangci-lint}"

which should work in dash too, or:

if echo "$gcil" | grep '/snap/bin/' ; then

(without ^ so that it matches both /snap/bin and /var/lib/snapd/snap/bin)

olivercalder · 2024-09-10T23:08:05Z

run-checks

        fi
-        goci_go_ver=$(golangci-lint --version | grep -o 'go[0-9]*\.[0-9]*\.[0-9]*' | cut -c 3-)
+
+        # Check whether the version golangci-lint was built with is >= version of go installed


Nittier nitpick:

Suggested change

# Check whether the version golangci-lint was built with is >= version of go installed

# Check whether golangci-lint was built with go version >= the installed go version

bboozzoo

Thanks

maykathm · 2024-09-17T07:33:22Z

Failed tests are:
openstack:fedora-40-64:tests/main/selinux-clean
google:ubuntu-20.04-64:tests/main/nvidia-files:535_server

selinux-clean is a known problem
nvidia-files:535_server fails because of the problem that was fixed after this pull request opened.

* run-checks: added golangci-lint run * run-checks: review corrections - remove superfluous exit * run-checks: review corrections * run-checks: added check for go version of golangci-lint and go * tests: added golagnci-lint skip in spread test * run-checks: review corrections - run golangci-lint also on older go and made clearer env variable and comment * run-checks: review corrections - allow for non-snap installation of golangci-lint * run-checks: review corrections

maykathm force-pushed the SNAPDENG-8502_add_golangci-lint branch 3 times, most recently from 0f04f51 to 36f9c0f Compare September 3, 2024 15:47

maykathm marked this pull request as ready for review September 3, 2024 16:10

maykathm requested a review from olivercalder September 3, 2024 16:10

sergiocazzolato reviewed Sep 3, 2024

View reviewed changes

run-checks Outdated Show resolved Hide resolved

sergiocazzolato reviewed Sep 3, 2024

View reviewed changes

run-checks Outdated Show resolved Hide resolved

olivercalder reviewed Sep 3, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

run-checks Outdated Show resolved Hide resolved

olivercalder reviewed Sep 5, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

olivercalder reviewed Sep 5, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

maykathm added 3 commits September 5, 2024 16:50

run-checks: added golangci-lint run

33eba24

run-checks: review corrections - remove superfluous exit

a35992d

run-checks: review corrections

20217de

maykathm force-pushed the SNAPDENG-8502_add_golangci-lint branch from 1b18954 to 20217de Compare September 5, 2024 14:50

maykathm added 2 commits September 5, 2024 18:08

run-checks: added check for go version of golangci-lint and go

98d6c5a

tests: added golagnci-lint skip in spread test

5509ae9

maykathm requested review from sergiocazzolato and olivercalder September 6, 2024 11:47

olivercalder reviewed Sep 6, 2024

View reviewed changes

run-checks: review corrections - run golangci-lint also on older go a…

3155b0c

…nd made clearer env variable and comment

maykathm requested review from olivercalder and bboozzoo September 10, 2024 09:53

bboozzoo reviewed Sep 10, 2024

View reviewed changes

run-checks: review corrections - allow for non-snap installation of g…

6ccd385

…olangci-lint

olivercalder approved these changes Sep 10, 2024

View reviewed changes

run-checks: review corrections

4540042

maykathm requested a review from bboozzoo September 16, 2024 14:37

bboozzoo approved these changes Sep 17, 2024

View reviewed changes

ernestl merged commit 91bc7cb into canonical:master Sep 17, 2024
51 of 54 checks passed

maykathm deleted the SNAPDENG-8502_add_golangci-lint branch September 17, 2024 15:13

olivercalder mentioned this pull request Sep 19, 2024

run-checks: add golangci-lint to static checks #13042

Closed

maykathm mentioned this pull request Oct 16, 2024

run-checks: run golangci-lint on all files #14626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run-checks: added golangci-lint to run-checks #14464

run-checks: added golangci-lint to run-checks #14464

maykathm commented Sep 3, 2024 •

edited

Loading

olivercalder left a comment

olivercalder left a comment

olivercalder left a comment

olivercalder left a comment

olivercalder Sep 6, 2024

olivercalder Sep 6, 2024

maykathm Sep 9, 2024

olivercalder Sep 6, 2024

olivercalder Sep 6, 2024

bboozzoo Sep 10, 2024

maykathm Sep 10, 2024

olivercalder left a comment

olivercalder Sep 10, 2024

bboozzoo Sep 11, 2024

olivercalder Sep 10, 2024 •

edited

Loading

bboozzoo left a comment

maykathm commented Sep 17, 2024

	echo "WARNING: Your go version ($go_ver) is greater than the version of go that golangci-lang was built with ($goci_go_ver)."
	echo "WARNING: Your go version ($go_ver) is greater than the version of go that golangci-lint was built with ($goci_go_ver)."

		# To avoid checking old code, skip golangci-lint
		skip="${skip:-} SKIP_GOLANG_CI_LINT=1"

	if [ "${gcil/\/snap\/bin/}" != "$gcil" ]; then
	if echo "$gcil" \| grep '^\/snap\/bin\/' ; then

	# Check whether the version golangci-lint was built with is >= version of go installed
	# Check whether golangci-lint was built with go version >= the installed go version

run-checks: added golangci-lint to run-checks #14464

run-checks: added golangci-lint to run-checks #14464

Conversation

maykathm commented Sep 3, 2024 • edited Loading

olivercalder left a comment

Choose a reason for hiding this comment

olivercalder left a comment

Choose a reason for hiding this comment

olivercalder left a comment

Choose a reason for hiding this comment

olivercalder 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

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

maykathm commented Sep 17, 2024

maykathm commented Sep 3, 2024 •

edited

Loading

olivercalder Sep 10, 2024 •

edited

Loading