refactor: code review fixes — dead code, error wrapping, deduplication#313
Merged
refactor: code review fixes — dead code, error wrapping, deduplication#313
Conversation
- Fix file handle leak in bundler handler Walk callback (explicit close after io.Copy) - Fix data race in recipe metadata store (remove unsynchronized pre-check before sync.Once) - Fix partial state mutation in sysctl parseMultiLineKeyValue (use temp map) - Replace os.ReadFile with streaming SHA256 in checksum generation/verification - Remove unused recipeCacheHits metric (caller removed in metadata store fix) - Fix snapshot test connecting to live cluster (use mock factory)
- Add new constants to pkg/defaults (AgentJobActiveDeadline, FileParserMaxSize, server rate limits) - Wrap 4 bare error returns in k8s/agent/rbac.go with errors.Wrap - Replace magic literals in k8s/agent/job.go with defaults constants - Replace 500ms poll interval literals with defaults.PodPollInterval - Add ctx.Done() check in GetPodLogs scan loop (matching StreamLogs pattern) - Wrap bare ctx.Err() returns across all collector packages (10 instances) - Remove unreachable guard in k8s/node.go parseProvider - Replace magic literal in file collector with defaults.FileParserMaxSize - Replace magic literals in server config/middleware with defaults constants - Add slog.Warn for silent env var parse failures in server config - Use clusterRoleName constant instead of string literal in rbac.go
Packages: errors, header, version, logging, manifest, snapshotter, k8s/agent, server, oci - Delete ExitCodeFromSignal, ExitFlagError, ExitSignalBase (zero callers) - Unexport ExitCodeFromErrorCode (only internal caller) - Delete KindValidationResult, ErrInvalidPrecision (zero references) - Delete Snapshotter interface (never implemented) - Delete WithConfig option (zero external callers) - Unexport 20+ symbols only used within their packages: HTTPStatusFromCode, ErrorResponse, WriteError, DefaultAPIVersion, SetAPIVersionHeader, HealthResponse, Config, HealthResponse, ArtifactType, ReproducibleTimestamp, ValidateRegistryReference, URIScheme, APIDomain, APIVersion, EnvVarLogLevel, LogPrefixEnvVar, NewCLIHandler, HelmFuncMap, PermissionCheck, header.New
…onent, evidence - Unexport recipe validation helpers (ValidateMeasurementExists, ValidateSubtypeExists, etc.) - Delete MustGetComponentRegistry (panic-on-error, no external callers) - Delete DefaultRecipeCacheTTL re-export - Unexport measurement helpers only used in tests (FilterIn, ToReadingWithType, etc.) - Unexport serializer NewFileReaderAuto - Unexport component helpers (ComputeChecksum, ExtractCustomLabels, etc.) - Unexport evidence types (EvidenceEntry, CheckEntry, RequirementMeta) - Cache parsed templates in evidence renderer (parse once, reuse) - Delete GetOwnPodTolerations (zero callers) - Export MatchesCriteriaField from recipe for cross-package use
…ance - Extract shared prepareCluster helper in validator (ValidatePhases + ValidatePhase) - Extract truncateLogLines helper in validator/job/result.go - Deduplicate matchesCriteriaField (use exported recipe.MatchesCriteriaField) - Move waitForDeletion to conformance/helpers.go (shared utility) - Use package-level resourceSliceGVR in conformance/helpers.go
lalitadithya
approved these changes
Mar 9, 2026
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (3 decrease, 10 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
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
Cleanup across the entire codebase. All changes verified with
make qualify(tests + lint + e2e + vuln scan + license headers).os.ReadFile+sha256.Sum256with streamingio.Copyto bound memory usageerrors.Wrapfor better diagnostics (k8s/agent/rbac, all collector ctx.Err paths)pkg/defaultsExitCodeFromSignal,MustGetComponentRegistry,Snapshotterinterface,WithConfig,GetOwnPodTolerations,KindValidationResult,ErrInvalidPrecision,BoolToString, unused metricprepareClusterin validator,truncateLogLinesin job/result,deployAndWaitForResultin snapshotter/agent, consolidatematchesCriteriaField, movewaitForDeletionto shared helpersctx.Done()check inGetPodLogsscan loop (matchingStreamLogspattern)slog.Warnfor silent env var parse failures in server configTest plan
make qualifypasses (tests + lint + e2e + vuln scan + license headers)go build ./...,go vet ./...)