refactor: replace magic literals with named constants#593
Merged
Conversation
…and pkg/errors Addresses findings from a full codebase review: - Replace fmt.Errorf in pkg/logging with errors.Wrap - Wrap bare sentinel error returns in pkg/version with error codes - Extract 11 named constants to pkg/defaults (timeouts, limits, K8s namespaces) - Replace hardcoded timeout values with defaults constants across validators - Replace hardcoded K8s resource names with defaults.GPUOperatorNamespace - Fix context cancellation race in sysctl_test.go
Coverage Report ✅
Coverage BadgeCoverage unchanged by this PR. |
lalitadithya
approved these changes
Apr 16, 2026
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
2 participants
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.
Summary
Replace magic literals,
fmt.Errorf, and hardcoded K8s resource names with named constants frompkg/defaultsand structured errors frompkg/errors. Fix a context cancellation race in a test.Motivation / Context
Full codebase review identified inconsistencies with project coding standards: magic numbers instead of named constants,
fmt.Errorfinstead ofpkg/errors, and hardcoded K8s namespace strings duplicated across validators.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
pkg/defaults/timeouts.go: Added 11 named constants — timeouts (RuntimeClassCheckTimeout,CNCFSubmissionTimeout,TrainerControllerPollInterval,TrainingRuntimePollInterval), output limits (TerminationLogMaxSize,ConfigMapStatusTruncateLen,AutoscalerMaxEvents,MetricsDisplayLimit), and K8s resource names (GPUOperatorNamespace,KubeSystemNamespace)pkg/logging/cli.go: Replacedfmt.Errorfwitherrors.Wrap(errors.ErrCodeInternal, ...)pkg/version/version.go: Wrapped bare sentinel returns (ErrEmptyVersion,ErrTooManyComponents) withpkgerrors.Wrap+ error codes; sentinels preserved forerrors.Is()compatibility"gpu-operator"and"kube-system"namespace strings withdefaults.GPUOperatorNamespace/defaults.KubeSystemNamespacesysctl_test.go: Added 10ms sleep before context cancellation to fix race wherecancel()fired beforeCollect()could startTesting
All tests pass (74.7% coverage, threshold 70%). Lint clean on all changed packages. Pre-existing gosec G703 in
argocdhelm.gois unrelated and exists on main.Risk Assessment
Rollout notes: N/A — purely mechanical refactoring with no behavioral changes.
Checklist
make testwith-race)make lint)git commit -S)