fix(feature): resolve lint failures in feature package#221
Conversation
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughThis PR refactors the ChangesFeature Extension Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fix goconst, gosec, cyclop, lll, modernize, revive, and funlen lint issues introduced by the OCI annotations PR. Refactors GetExtendedBuildInfo to use a params struct (revive argument-limit), extracts helper functions from findContainerUsers (cyclop), uses strings.Builder in getFeatureLayers (modernize), and adds filepath.Clean to os.Open calls (gosec G304).
da0df62 to
07e31dc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/devcontainer/feature/extend.go (1)
260-301: 💤 Low valueHelper extraction reads well.
Splitting metadata-derived users from fallback resolution makes the precedence (metadata → compose service user → image user) explicit and unit-testable. Behavior is unchanged.
One small note:
usersFromMetadatadereferencesmeta.Configwithout a nil guard onmeta. All current call paths populateimageBuildInfo.Metadataviametadata.GetImageMetadata, so this isn't reachable today, but a defensiveif meta == nil { return "", "" }would harden the helper against future callers. Optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/feature/extend.go` around lines 260 - 301, Add a nil-check at the top of usersFromMetadata to avoid dereferencing meta when nil: if meta == nil { return "", "" }, because usersFromMetadata currently calls config.ReverseSlice(meta.Config) and will panic if meta is nil; update the function (usersFromMetadata) to return empty containerUser and remoteUser when meta is nil so callers like findContainerUsers remain safe.pkg/devcontainer/feature/features.go (1)
419-464: 💤 Low valueIntegrity check flow looks reasonable; one consideration on backward-compat behavior.
verifyCacheIntegrityreturningtruewhen thefeature.sha256sidecar is missing is appropriate for caches populated before this change. Just be aware that ifstoreIntegrityHashever fails to write the sidecar (only logged at error level), subsequent runs will silently fall back to the backward-compat path and skip verification for that cache entry indefinitely. If you want stricter enforcement once a feature has been re-downloaded post-upgrade, consider treating a write failure here as a signal to invalidate the cache (e.g., removefeature.tgz) so the next run re-downloads and re-attempts the sidecar write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/feature/features.go` around lines 419 - 464, The current flow lets verifyCacheIntegrity accept missing sidecars; to avoid silently skipping integrity after storeIntegrityHash fails to write the sidecar, update storeIntegrityHash to treat a failed os.WriteFile as a cache-invalidation event: on write failure (in storeIntegrityHash) delete or move the corresponding tarball (tarballPath or filepath.Join(featureFolder,"feature.tgz")) so the cached artifact is removed and the next run must re-download and reattempt writing the sidecar; keep the error log but also add a log entry indicating the cache invalidation action and ensure verifyCacheIntegrity remains unchanged.pkg/devcontainer/feature/extend_test.go (1)
10-14: ⚡ Quick winConsider extending the const block to consolidate repeated test feature IDs.
The new
testFeatureA/B/Cconstants clean up duplication effectively. However, the literal"ghcr.io/devcontainers/features/node"appears 20 times throughout the file and triggers//nolint:goconstsuppressions at lines 63, 572, 635, 645, 676, and 689. AddingtestFeatureNodeRef = "ghcr.io/devcontainers/features/node"to the const block would eliminate these suppressions and improve maintainability.Additionally,
testFeatureNodeis used in this file (lines 56–71) but defined incollection_test.go. While both files are in the same package and the constant is accessible, addingtestFeatureNode = "node"to the local const block would make the dependency explicit and match the pattern oftestFeatureA/B/C.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/feature/extend_test.go` around lines 10 - 14, The const block defining testFeatureA/B/C should be extended to include testFeatureNodeRef = "ghcr.io/devcontainers/features/node" and testFeatureNode = "node" so the repeated literal and external dependency are declared locally; update references in tests to use these new constants (replacing the 20 literal occurrences and the uses that currently rely on testFeatureNode from collection_test.go) and then remove the corresponding //nolint:goconst suppressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/devcontainer/feature/extend_test.go`:
- Around line 10-14: The const block defining testFeatureA/B/C should be
extended to include testFeatureNodeRef = "ghcr.io/devcontainers/features/node"
and testFeatureNode = "node" so the repeated literal and external dependency are
declared locally; update references in tests to use these new constants
(replacing the 20 literal occurrences and the uses that currently rely on
testFeatureNode from collection_test.go) and then remove the corresponding
//nolint:goconst suppressions.
In `@pkg/devcontainer/feature/extend.go`:
- Around line 260-301: Add a nil-check at the top of usersFromMetadata to avoid
dereferencing meta when nil: if meta == nil { return "", "" }, because
usersFromMetadata currently calls config.ReverseSlice(meta.Config) and will
panic if meta is nil; update the function (usersFromMetadata) to return empty
containerUser and remoteUser when meta is nil so callers like findContainerUsers
remain safe.
In `@pkg/devcontainer/feature/features.go`:
- Around line 419-464: The current flow lets verifyCacheIntegrity accept missing
sidecars; to avoid silently skipping integrity after storeIntegrityHash fails to
write the sidecar, update storeIntegrityHash to treat a failed os.WriteFile as a
cache-invalidation event: on write failure (in storeIntegrityHash) delete or
move the corresponding tarball (tarballPath or
filepath.Join(featureFolder,"feature.tgz")) so the cached artifact is removed
and the next run must re-download and reattempt writing the sidecar; keep the
error log but also add a log entry indicating the cache invalidation action and
ensure verifyCacheIntegrity remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ada0723-026c-440e-9bd3-c29fab4460f2
📒 Files selected for processing (7)
pkg/devcontainer/build.gopkg/devcontainer/compose.gopkg/devcontainer/feature/annotations_test.gopkg/devcontainer/feature/collection_test.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/extend_test.gopkg/devcontainer/feature/features.go
1 participant
Summary
Fixes goconst, gosec, cyclop, lll, modernize, revive, and funlen lint failures introduced in the OCI annotations PR (#218). Refactors
GetExtendedBuildInfoto use a params struct to satisfy the revive argument-limit rule, extracts helper functions fromfindContainerUsersto reduce cyclomatic complexity, usesstrings.BuilderingetFeatureLayers, and appliesfilepath.Cleantoos.Opencalls for gosec compliance.Summary by CodeRabbit
Bug Fixes
Refactor