Skip to content

Conversation

@vikin91
Copy link
Contributor

@vikin91 vikin91 commented Dec 12, 2025

What

Updated quickstyle to be more reliable in StackRox repos by resolving Go tooling via repo-managed binaries when available and by ensuring required tools exist.

Why

Running quickstyle against stackrox repo could fail for non-style reasons.

Before this change

  • Stale roxvet from GOPATH/bin: quickstyle would use $(go env GOPATH)/bin/roxvet if present. If it was built with an older Go version than the target repo’s toolchain, it could error with “package requires newer Go version …” and/or panic (e.g. pkgbits unsupported version: 2).
  • Missing goimports: quickstyle called goimports directly without ensuring it exists, so environments without goimports on PATH failed with goimports: command not found.
  • Repo toolchain not respected: repos like stackrox manage tools under .gotools/bin (via make which-*), but quickstyle didn’t prefer those canonical binaries, so results depended on whatever was installed globally.

This change makes quickstyle prefer repo toolchain wiring (via make which-*) and provides a workflow-managed fallback for goimports.

Changes

  • Prefer repo-provided tool binaries via make which-<tool> when available (e.g. gotools-based roxvet, golangci-lint).
  • Detect stale GOPATH/bin/roxvet (Go major/minor mismatch) and rebuild it when possible.
  • Ensure goimports is available by installing it into a workflow cache (pinned to the repo’s golang.org/x/tools version when available) and reusing it.
  • Add doc comments for the new helper functions for maintainability.

How I validated my change

  • Reproduced the failure mode in stackrox by running:
    • cd ../stackrox && ../workflow/bin/quickstyle
  • Verified the updated script:
    • no longer fails with goimports: command not found
    • no longer panics due to stale roxvet (and uses/rebuilds a compatible roxvet)
    • still reports real style findings from the repo (expected).

Example run after the changes:

$ pushd ../stackrox2 && quickstyle || popd

~/src/go/src/github.com/stackrox/stackrox2 ~/src/go/src/github.com/stackrox/workflow
[INFO] 10 files changed from master, 10 changed since the last successful quickstyle run.
[INFO] Adding missing newlines...
[INFO] Running go style checks...
[INFO] golangci-lint
sensor/kubernetes/fake/virtualmachines.go:337:70: printf: (github.com/stackrox/rox/pkg/logging.Logger).Debugf format %s has arg vsockCID of wrong type uint32 (govet)
                        log.Debugf("Stopping index report generation for VM with vsockCID %s (lifecycle ended)", vsockCID)
                                                                                          ^
1 issues:
* govet: 1
[INFO] imports
[INFO] blanks
[INFO] fmt
[INFO] roxvet
[INFO] staticcheck
[WARN] Skipping staticcheck, doesn't appear to be supported in the repo.
[FATAL] Style errors were found
~/src/go/src/github.com/stackrox/workflow

AI / human attribution

  • AI: implemented the tool-resolution logic (make which-* + fallbacks), goimports caching/installation behavior, and helper-function documentation.
  • Human: provided requirements, executed the reproduction/validation runs, and reviewed/accepted the changes.

quickstyle now prefers repo-managed tool binaries via `make which-*` (e.g. gotools), avoiding failures in stackrox2 when PATH/GOPATH contain stale tools (notably roxvet panics due to Go version mismatch). It also auto-installs goimports (pinned to the repo’s golang.org/x/tools version when available) into a workflow cache so “goimports: command not found” doesn’t break runs, and documents the helper functions for maintainability.

AI: implemented the tool-resolution logic, caching/install behavior, and the added helper-function documentation.
User: provided the requirements, validated behavior by running quickstyle, and reviewed/accepted the change.

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@vikin91
Copy link
Contributor Author

vikin91 commented Dec 12, 2025

This change is part of the following stack:

Change managed by git-spice.

@vikin91 vikin91 marked this pull request as ready for review December 12, 2025 11:14
@janisz janisz requested a review from guzalv December 16, 2025 13:11
Copy link

@janisz janisz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested! It's working for me:

before I have a lot of similar panics

# go.uber.org/zap/internal/exit
panic: assertion failed [recovered]
	panic: assertion failed

now:

quickstyle    
[INFO] 4 files changed from master, 4 changed since the last successful quickstyle run.
[INFO] Adding missing newlines...
[INFO] Running go style checks...
[INFO] golangci-lint
0 issues.
[INFO] imports
[INFO] blanks
[INFO] fmt
[INFO] roxvet
[INFO] staticcheck
[WARN] Skipping staticcheck, doesn't appear to be supported in the repo.

@vikin91 vikin91 merged commit da029b0 into master Jan 2, 2026
1 check passed
@vikin91 vikin91 deleted the piotr/resurrect-quickstyle branch January 2, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants