hyperledger / hyperledger/fabric-x-sdk

Static analysis in CI

Open
#44 0 comments 0 reactions 0 assignees View on GitHub
good-first-issue
Dominant language
Go
Stars
3
Forks
5
Avg merge
2d 11h
Merged PRs (30d)
9

Description

## Context

CI today runs exactly one gate (`.github/workflows/tests.yml`'s "Checks" step, `make checks` —
Makefile:1-5): `gofmt -l -s`, `go vet -all`, and `addlicense -check`. There is no linter beyond
`go vet`, no security-focused static analysis, and no vulnerability scanning wired into CI at
all — `govulncheck` exists as a tool a developer could run locally but nothing re-runs it
automatically as dependencies change (it was run by hand during a production-readiness review this
session; came back clean, but nothing keeps it that way on the next dependency bump).

## Approach

### 1. `golangci-lint` as a `go tool` dependency

This repo already uses Go 1.24+'s `tool` block in go.mod for exactly this kind of thing —
`addlicense`, `configtxgen`, `cryptogen`, `fxconfig` are all wired up this way today (go.mod:5-10).
Add `golangci-lint` (`github.com/golangci/golangci-lint/v2/cmd/golangci-lint`) the same way: no new
install step for contributors (`go tool golangci-lint` just works once `go mod tidy` has fetched
it), and CI needs no new GitHub Action — the existing `go` setup step already resolves `tool`
dependencies from the module cache.

### 2. `.golangci.yml` — conservative config

Enable golangci-lint's own default linter set (`errcheck`, `gosimple`, `govet`, `ineffassign`,
`staticcheck`, `unused`) plus `gosec` for the security-review-shaped class of finding.
Start conservative, tighten later if wanted.

One concrete exclusion worth setting from the start: `errcheck` flags every unchecked
`fmt.Fprintf`/`fmt.Fprintln` return value, and this repo already has a cluster of these in
`state/bench_test.go`'s console benchmark-report helper (writing to a `tabwriter.Writer`/stdout —
the canonical "not worth checking" errcheck case, which is why the standalone `errcheck` tool ships
a default exclusion list covering exactly this). Configure errcheck's `exclude-functions` for the
`fmt.Fprint*` family up front rather than either hand-suppressing each call site or letting this one
low-value pattern inflate the findings count and make the linter feel noisier than it is.

### 3. Fold into `make checks`

Add `go tool golangci-lint run ./...` as a fourth line in the existing `checks` target
(Makefile:1-5), keeping the established "one command, one gate" pattern — CI's existing "Checks"
step picks it up automatically with no new workflow step.

### 4. `govulncheck` as a second `go tool` dependency

Add `golang.org/x/vuln/cmd/govulncheck` the same way as (1). Add a `make vulncheck: go tool
govulncheck ./...` target, called as its own step in the CI "Checks" job (kept separate from `make
checks` rather than folded in — a new CVE disclosure in a transitive dependency is a different kind
of failure than a formatting/vet regression in this PR's diff, and is worth being separately
attributable in CI output).

### 5. Triage and fix the initial findings wave

Turning on a linter after the fact on an existing codebase always surfaces a first wave of
findings. Given the conservative linter set chosen in (2) and this codebase's existing discipline
(consistent error wrapping, `//nolint:errcheck` already used deliberately in a few places, no
`TODO`/`FIXME` sprawl), the findings wave is not expected to be large. Fix it as part of this issue
rather than deferring: triage each finding and either fix it or suppress it with
`//nolint:` plus a short justification — the codebase already uses that suppression pattern
(e.g. on deferred `Close()` calls), so it's not a new convention to introduce. If a finding turns
out to be large/contentious (a real architectural issue, not a lint nit), split it out to its own
follow-up rather than blocking this PR on it.

## Out of scope

- Pinning exact `golangci-lint`/`govulncheck` versions — implementation detail, pin to current
stable at PR time.
- A stricter linter config beyond the conservative default+`gosec` set — natural follow-up once the
team has lived with the conservative set for a while.
- Any finding that turns out to be a real, non-trivial architectural issue rather than a lint nit —
split to its own follow-up rather than blocking this PR (see item 5).

## Verification

- `go tool golangci-lint run ./...` and `go tool govulncheck ./...` run clean locally, with zero
findings or only explicitly `//nolint`-justified ones — not just "wired up," actually clean.
- `make checks` still passes end to end.
- Push a throwaway PR that intentionally introduces one lint violation to confirm CI actually fails
on it (sanity-checks the wiring, not just that the tool runs), then revert the violation before
merging.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with go.mod:5-10, Makefile:1-5, and .github/workflows/tests.yml's Checks step to trace the existing tool and CI setup. Add the requested lint and vulnerability-check targets, configure the named conservative linters and exclusions, and triage initial findings. Done means both tools run cleanly, make checks passes, CI runs vulnerability checks separately, and an intentional lint failure is verified to block CI.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, go
Domain
ci-cd, devtools, security
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.