ci: add build, unit test, vet and lint workflows - #123
Open
bhatnitish wants to merge 2 commits into
Open
Conversation
`make lint` does not currently work in this repo. It fails before reporting
anything:
can't load config: the Go language version (go1.23) used to build
golangci-lint is lower than the targeted Go version (1.25.8)
[email protected] has a toolchain directive forcing it to be built with
go1.23.6, while go.mod targets Go 1.25.8, and golangci-lint refuses that
combination. Staying on v1 is not an option: every v1 release up to and
including the final v1.64.8 is built with Go <= 1.24.
Bump to v2.13.1 (built with go1.26.7). Note the module path gains a /v2 suffix.
There is no .golangci.yml in this repo, so there is no config to migrate - only
the default linter set changes slightly (gosimple folds into staticcheck).
Fix the 34 issues this surfaces. Note that golangci-lint caps duplicate
messages at 3 by default, which reports only 21 of them; the full set was found
with --max-same-issues=0.
errcheck 7 unchecked Close() / os.Setenv / os.Unsetenv returns
staticcheck 13 ST1005 capitalized error strings
staticcheck 5 QF1004 strings.Replace(..., -1) -> strings.ReplaceAll
staticcheck 4 QF1008 redundant embedded-field selectors
staticcheck 2 QF1003 if/else-if chain -> tagged switch
staticcheck 2 SA1019 deprecated NodeSystemInfo.KubeProxyVersion
staticcheck 1 ST1001 dot import
Notes on the two judgement calls:
- SA1019: KubeProxyVersion appears only as test fixture data and is never read
by non-test code, so the field is dropped rather than suppressed.
- ST1001: the dot import is gocheck's documented idiom and there are 66 call
sites across the e2e suite that would need qualifying. Suppressed with a
nolint carrying that rationale rather than churning the suite.
All changes are behaviour preserving. Verified: make manager, make vet, make
unit-test and make lint all pass, and lint reports 0 issues uncapped.
This repo has no build or test CI. Its only workflow, linting.yml, lints documentation via ROCm/rocm-docs-core. A pull request can currently break the build or the unit tests with no automated signal. Add four jobs, each mapping to an existing Makefile target so CI runs exactly what a developer runs locally: Build make manager + uploads the binary Unit Tests make unit-test + uploads cover.out Vet make vet Lint make lint The jobs are independent, so a failure is attributable without reading another job's log. Go comes from actions/setup-go reading go.mod, so the toolchain tracks the module rather than being pinned separately in CI. Vet is deliberately its own job even though `unit-test` depends on `vet` and therefore runs it too. The duplicate run costs seconds and means a vet failure is visible directly instead of being buried in the unit test output. Triggers on pull requests to main and release-*, pushes to main so a broken main announces itself immediately rather than at the next PR, and manual dispatch. The concurrency group cancels superseded runs. No required status checks are configured, so nothing here blocks merge. That is deliberate: it lands CI without disrupting in-flight work and builds the run history needed to decide what is stable enough to enforce later. The workflow declares `permissions: contents: read`, references no secrets and does not use pull_request_target, so fork pull requests are safe to run. Who may trigger a run is a repository setting (Actions > Fork pull request workflows from outside collaborators), not workflow configuration. Checkout does not fetch submodules. external/common-infra-operator is declared but vendored, and no Makefile target or Dockerfile references external/, so fetching it would only add a failure mode. Verified: actionlint reports no issues, all four targets pass, and the two uploaded paths - manager and cover.out - are produced where the workflow expects them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
This repo has no build or test CI. Its only workflow,
linting.yml, lints documentation viaROCm/rocm-docs-core. A PR can currently break the build or the unit tests with no automated signal.Jobs
Each maps to an existing Makefile target, so CI runs exactly what you run locally:
make managermanagerbinarymake unit-testcover.outmake vetmake lintFour independent jobs, no
needs:between them, so a failure is attributable without reading another job's log. Go comes fromactions/setup-gowithgo-version-file: go.mod, so the toolchain tracks the module rather than being pinned separately in CI.Vet is deliberately its own job even though
unit-test: vetalready runs it. The duplicate run costs seconds and makes a vet failure visible directly rather than buried in unit-test output. Say the word if you'd rather drop it.Triggers
PRs to
mainandrelease-*, pushes tomain(so a brokenmainannounces itself immediately rather than at the next PR), plus manual dispatch. Concurrency group cancels superseded runs.All checks are advisory
No required status checks are configured — nothing here blocks merge. Deliberate: it lands CI without any chance of disrupting in-flight work, and produces the run history needed to judge which jobs are stable enough to enforce. Enforcement is a follow-up and needs repo admin.
Fork safety
permissions: contents: read, no secrets, nopull_request_target. Note that who may trigger a run is a repository setting (Settings → Actions → General → Fork pull request workflows from outside collaborators), not workflow config.Submodules
Checkout deliberately does not fetch submodules.
external/common-infra-operatoris declared, but the dependency is vendored and no Makefile target or Dockerfile referencesexternal/— fetching it would only add a failure mode. (It would work if needed: the repo is public andactions/checkoutrewrites the SSH URL.)Verification
actionlintclean. All four targets run on this branch:make managermake unit-testmake vetmake lintI also confirmed the two uploaded paths —
managerandcover.out— are produced at the repo root where the workflow expects them, and that both are already gitignored.I can't run the workflow itself against this repo, so its first real execution will be on this PR.