chore: migrate to golangci-lint v2

[romshark]

This finally upgrades golangci-lint to v2 including changes to the code required by the new linter.

The most error-prone change is 2c009b0 (applying De Morgan's law in boolean expressions)

Depends on:

• refactor: scion-pki preparation for golangci-lint v2 #4771
• refactor: remove deprecated grpc api usage #4772
• refactor: remove deprecated bazel go api usage [DRAFT] #4773

[jiceathome]

This change is 

[katyatitkova]

I wrote some primitive checkers to see if the changes from 2c009b0 work, and they seem to be 100% correct. Here's the code corresponding to every change:

• control/beaconing/writer.go: https://go.dev/play/p/L0AO7P4Hj2D
• control/drkey/grpc/drkey_service.go: https://go.dev/play/p/Ne2D6Gp7abi
• pkg/private/serrors/errors.go: https://go.dev/play/p/4g00kJmNTCL
• private/mgmtapi/cppki/api/api.go: https://go.dev/play/p/WO0-T8XR5jQ
• router/bfd/session.go: https://go.dev/play/p/dmxZ3iLv6Ow
• router/dataplane.go: https://go.dev/play/p/tp2egIsl0ZC
• tools/end2end/main.go: https://go.dev/play/p/0Q5S3V5Lumq

[jiceathome]

May be QF1001 can be disabled from staticcheck. so we do not have to always bend to a machine's opinion regarding the legibility of boolean expressions?

Viewed in some else's config file:

    staticcheck:
      # SAxxxx checks in https://staticcheck.dev/docs/configuration/options/#checks
      # Example (to disable some checks): [ "all", "-SA1000", "-SA1001"]
      # Default: ["all", "-ST1000", "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022"]
      checks:
        - all
        # Poorly chosen identifier.
        # https://staticcheck.dev/docs/checks/#ST1003
        - -ST1003
        # The documentation of an exported function should start with the function's name.
        # https://staticcheck.dev/docs/checks/#ST1020
        - -ST1020
        # The documentation of an exported type should start with type's name.
        # https://staticcheck.dev/docs/checks/#ST1021
        - -ST1021
        # The documentation of an exported variable or constant should start with variable's name.
        # https://staticcheck.dev/docs/checks/#ST1022
        - -ST1022
        # Apply De Morgan's law.
        # https://staticcheck.dev/docs/checks/#QF1001
        - -QF1001

[romshark]

I'll keep the changes to the existing expressions since some of them are both IMHO and in @katyatitkova opinion more readable now (less unnecessary negation) but I'll disable QF1001

[jiceathome]

Other than my suggestion rgarding QF1001, LGTM.

[lukedirtwalker]

Reviewed 1 of 3 files at r1, 46 of 50 files at r2, 12 of 12 files at r3, 9 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion, @oncilla, and @romshark)

---

router/bfd/session.go line 538 at r5 (raw file):

	}
	if pkt.YourDiscriminator == 0 {
		if pkt.State != layers.BFDStateAdminDown && pkt.State != layers.BFDStateDown {

Suggestion:

	if pkt.YourDiscriminator == 0 && pkt.State != layers.BFDStateAdminDown && pkt.State != layers.BFDStateDown {

---

router/bfd/session.go line 543 at r5 (raw file):

	}
	if !pkt.AuthPresent {
		if pkt.AuthHeader != nil && pkt.AuthHeader.AuthType != layers.BFDAuthTypeNone {

Suggestion:

	if !pkt.AuthPresent && pkt.AuthHeader != nil && pkt.AuthHeader.AuthType != layers.BFDAuthTypeNone {

---

private/path/pathpol/acl.go line 190 at r5 (raw file):

	case denySymbol:
		return false, nil
	}

I would just put this in the default case

[romshark]

private/path/pathpol/acl.go line 190 at r5 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I would just put this in the default case

you mean putting serrors.New in a default case? I don't see a benefit since it's just 1 extra line that does the exact same. 🤷‍♂️

[lukedirtwalker]

Reviewed 7 of 7 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jiceatscion, @oncilla, and @romshark)

[romshark]

I'll keep the changes to the existing expressions since some of them are both IMHO and in @katyatitkova opinion more readable now (less unnecessary negation) but I'll disable QF1001

[romshark]

.golangcilint.yml line at r3 (raw file):

Previously, jiceatscion wrote…

May be it would help the review of the new file if this one was git mv'd to the new name first?

I don't think that's necessary since the config structure in v2 changed anyway so the diff would probably be just as bad as comparing a new and old config file.

[romshark]

router/bfd/session.go line 538 at r5 (raw file):

	}
	if pkt.YourDiscriminator == 0 {
		if pkt.State != layers.BFDStateAdminDown && pkt.State != layers.BFDStateDown {

Done.

[romshark]

router/bfd/session.go line 543 at r5 (raw file):

	}
	if !pkt.AuthPresent {
		if pkt.AuthHeader != nil && pkt.AuthHeader.AuthType != layers.BFDAuthTypeNone {

Done.

[lukedirtwalker]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion, @katyatitkova, and @oncilla)

[romshark]

checked diff, LGTM, can merge

[lukedirtwalker]

Reviewed 21 of 21 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jiceatscion, @katyatitkova, and @oncilla)

---

.golangci.yml line 30 at r8 (raw file):

        # The use of Go ideomatic identifiers is a recommendation, not a law.
        - "-ST1003" # disable "Poorly chosen identifier".
        - "-QF1008"	# disable "Omit embedded fields from selector expression".

tab vs space?

Code quote:

	# 

[romshark]

.golangci.yml line 30 at r8 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

tab vs space?

indeed, good catch! Done.

[jiceathome]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)

[lukedirtwalker]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)