Skip to content

fix: resolve goconst and errorlint warnings across OCI, coverage, validators#1916

Open
andrewwhitecdw wants to merge 4 commits into
NVIDIA:mainfrom
andrewwhitecdw:fix/lint-goconst-errorlint-sweep
Open

fix: resolve goconst and errorlint warnings across OCI, coverage, validators#1916
andrewwhitecdw wants to merge 4 commits into
NVIDIA:mainfrom
andrewwhitecdw:fix/lint-goconst-errorlint-sweep

Conversation

@andrewwhitecdw

Copy link
Copy Markdown

Summary

Fixes four small lint/test issues surfaced by golangci-lint and static analysis: one errorlint violation in OCI push tests and three goconst duplications in coverage tooling and validator packages.

Motivation / Context

N/A — opportunistic cleanup from a lint/test sweep.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • Build/CI/tooling

Component(s) Affected

  • CLI (cmd/aicr, pkg/cli)
  • API server (cmd/aicrd, pkg/server)
  • Recipe engine / data (pkg/recipe)
  • Bundlers (pkg/bundler, pkg/component/*)
  • Collectors / snapshotter (pkg/collector, pkg/snapshotter)
  • Validator (pkg/validator)
  • Core libraries (pkg/errors, pkg/k8s)
  • Docs/examples (docs/, examples/)
  • Other: tools/coverage

Implementation Notes

  • Replaced err == io.EOF with stderrors.Is(err, io.EOF) in pkg/oci/push_test.go.
  • Centralized the em dash placeholder as emDash in tools/coverage.
  • Added allocationModeExactCount in validators/conformance/secure_access_check.go.
  • Added skipMsgNCCLFewNodes in validators/performance/nccl_all_reduce_bw_constraint.go and updated tests.

Testing

# Targeted package tests and lint on changed packages:
go test ./pkg/oci/... ./tools/coverage/... ./validators/conformance/... ./validators/performance/...
golangci-lint run -c .golangci.yaml ./pkg/oci/... ./tools/coverage/... ./validators/conformance/... ./validators/performance/...

All targeted tests pass and golangci-lint reports 0 issues. Full make lint reached the Go lint step successfully; the overall target stopped at the YAML lint step because yamllint is not installed in this environment.

Risk Assessment

  • Low — Isolated change, well-tested, easy to revert
  • Medium — Touches multiple components or has broader impact
  • High — Breaking change, affects critical paths, or complex rollout

Rollout notes: N/A

Checklist

  • Tests pass locally (make test with -race)
  • Linter passes (make lint)
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S) — GPG signing info

Replaces a direct err == io.EOF comparison with stderrors.Is(err, io.EOF) so

the test follows the project errorlint rules and handles wrapped EOF correctly.

Signed-off-by: Andrew White <andrewh@cdw.com>
Introduces emDash in model.go and replaces repeated literal em dash strings

in build.go and render.go to satisfy the goconst linter.

Signed-off-by: Andrew White <andrewh@cdw.com>
Adds allocationModeExactCount and uses it for both v1beta1 and newer

ResourceClaim request shapes, resolving a goconst warning.

Signed-off-by: Andrew White <andrewh@cdw.com>
Adds skipMsgNCCLFewNodes and uses it in the constraint and its tests to

eliminate the goconst duplication.

Signed-off-by: Andrew White <andrewh@cdw.com>
@andrewwhitecdw
andrewwhitecdw requested review from a team as code owners July 25, 2026 02:08
@copy-pr-bot

copy-pr-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

Welcome to AICR, @andrewwhitecdw! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off per the DCO
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: ac8626e6-2ded-425e-b7b4-7b96428eb4c2

📥 Commits

Reviewing files that changed from the base of the PR and between 7e0f4a1 and 5204c9c.

📒 Files selected for processing (8)
  • pkg/oci/push_test.go
  • tools/coverage/build.go
  • tools/coverage/model.go
  • tools/coverage/render.go
  • validators/conformance/secure_access_check.go
  • validators/performance/nccl_all_reduce_bw_constraint.go
  • validators/performance/nccl_benchmark_profile_test.go
  • validators/performance/nccl_benchmark_runtime_test.go

📝 Walkthrough

Walkthrough

The changes centralize repeated em-dash, allocation-mode, and NCCL skip-message strings into package constants, updating their production and test usages. OCI tar iteration now detects EOF with wrapped-error matching instead of direct error equality. No exported entities are changed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: mchmarny

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the lint cleanup across OCI, coverage, and validator code.
Description check ✅ Passed The description is directly related to the changes and matches the implementation and testing details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@mohityadav8 mohityadav8 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! I fetched this locally to test the changes. I ran golangci-lint against pkg/oci/, tools/coverage/, and the validator packages. I can confirm the previous goconst and errorlint warnings are completely resolved.
LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants