Upgrade golangci lint to v2 #562
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure WalkthroughRepository-wide updates include expanded linting configuration and CI action version bumps, pinned tool versions in the Makefile, relocation of priority name-to-ID mapping, template and JSON Schema struct expansions, added context timeouts to several tests and a generator, minor refactors/formatting across various files, and small schedule handler message/annotation tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Generator as restic/generator.install
participant OSExec as exec.CommandContext
participant Restic as restic manpage proc
User->>Generator: run install()
note over Generator: Create context with 10m timeout
Generator->>OSExec: CommandContext(ctx, "restic", "man", ...)
OSExec-->>Restic: spawn process
alt completes before timeout
Restic-->>OSExec: exit 0
OSExec-->>Generator: return nil
Generator-->>User: done
else exceeds timeout / ctx cancelled
OSExec-->>Generator: context deadline exceeded error
Generator-->>User: propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/info.go (2)
332-337: numberValuePattern rejects valid numbers like “12.3”Pattern
^-?\d?\.?\d+$allows at most one digit before the decimal; “12.3” fails and such options are mis‑typed as string. Fix the regex to accept common forms.Apply:
-var ( - numberValuePattern = regexp.MustCompile(`^-?\d?\.?\d+$`) +var ( + // Accepts integers and decimals with optional leading minus and optional leading zero digits, e.g. "123", "-5", "12.3", ".5", "0.5" + numberValuePattern = regexp.MustCompile(`^-?\d*\.?\d+$`) intValuePattern = regexp.MustCompile(`^-?\d+$`) boolValuePattern = regexp.MustCompile(`^(true|false)$`)Alternatively, avoid regex and detect with strconv.ParseFloat for robustness.
382-394: Nested PropertySet name set before tags → empty NameIn configurePropertyFromType you use p.Name() to set the nested set’s name, but p.name is only populated later by configureBasicPropertyFromTags (via mapstructure tag). Result: nested name can be empty.
Set basic tags first (for the name), then determine the type, then complete tag‑based config:
- p := propertyInfo{originField: &field} - configurePropertyFromType(p.basic(), field.Type) - configurePropertyFromTags(&p, &field) + p := propertyInfo{originField: &field} + // Ensure name is available before building nested sets + configureBasicPropertyFromTags(&p.basicPropertyInfo, &field) + configurePropertyFromType(p.basic(), field.Type) + // Now apply full tag configuration (defaults, examples, enums, etc.) + configurePropertyFromTags(&p, &field)This preserves default/enum logic (which depends on type) and ensures nested.Name is correctly populated.
🧹 Nitpick comments (9)
.golangci.yml (1)
72-89: Tighten path regexes in exclusions.Anchor the start to avoid accidental matches in nested paths.
Apply:
paths: - - third_party$ - - builtin$ - - examples$ + - ^third_party$ + - ^builtin$ + - ^examples$Repeat the same anchoring under formatters.exclusions.paths.
config/jsonschema/model.go (2)
398-409: Also run base type checks in schemaString.verify().Currently string schemas skip schemaTypeBase.verify(), so invalid/empty type metadata wouldn’t be caught.
Apply:
func (s *schemaString) verify() (err error) { if len(s.Pattern) > 0 { if err = verifySchemaRegex(s.Pattern); err != nil { err = fmt.Errorf("pattern regex %q failed to compile: %w", s.Pattern, err) } } if !slices.Contains(validFormatNames, s.Format) { err = fmt.Errorf("format %q is no valid string format", s.Format) } - return + if err == nil { + err = s.schemaTypeBase.verify() + } + return }
323-327: Consider omitting zero-value constraints to reduce schema noise.minItems/minLength=0 and uniqueItems=false are equivalent to being absent; omitting them makes schemas leaner.
Apply:
type schemaArray struct { schemaTypeBase - Items SchemaType `json:"items"` - MinItems uint64 `json:"minItems"` - MaxItems *uint64 `json:"maxItems,omitempty"` - UniqueItems bool `json:"uniqueItems"` + Items SchemaType `json:"items"` + MinItems uint64 `json:"minItems,omitempty"` + MaxItems *uint64 `json:"maxItems,omitempty"` + UniqueItems bool `json:"uniqueItems,omitempty"` } type schemaString struct { schemaTypeBase - MinLength uint64 `json:"minLength"` + MinLength uint64 `json:"minLength,omitempty"` MaxLength *uint64 `json:"maxLength,omitempty"` ContentEncoding string `json:"contentEncoding,omitempty"` ContentMediaType string `json:"contentMediaType,omitempty"` Pattern string `json:"pattern,omitempty"` Format stringFormat `json:"format,omitempty"` }Also applies to: 390-396
lock/lock_test.go (3)
259-266: Use CommandContext here too to avoid potential hangs.Align with TestMain and bind the helper to a timeout.
Apply:
- cmd := exec.Command(helperBinary, "-wait", "1", "-lock", lockfile) + ctx, cancel := context.WithTimeout(context.Background(), constants.DefaultTestTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, helperBinary, "-wait", "1", "-lock", lockfile)
279-286: Bind long-running helper to a context.Prevents stray processes if signalling fails.
Apply:
- cmd := exec.Command(helperBinary, "-wait", "2000", "-lock", lockfile) + ctx, cancel := context.WithTimeout(context.Background(), constants.DefaultTestTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, helperBinary, "-wait", "2000", "-lock", lockfile)
306-316: Context for shell-wrapped helper as well.Same rationale as above.
Apply:
- cmd := exec.Command("sh", "-c", "exec "+helperBinary+" -wait 2000 -lock "+lockfile) + ctx, cancel := context.WithTimeout(context.Background(), constants.DefaultTestTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "sh", "-c", "exec "+helperBinary+" -wait 2000 -lock "+lockfile)commands_generate.go (1)
80-89: Avoid shadowing the imported templates package.Renaming the local FS variable improves readability.
Apply:
- templates, err := fs.Sub(configReferenceTemplates, pathTemplates) + tmplFS, err := fs.Sub(configReferenceTemplates, pathTemplates) ... - tpl, err = tpl.ParseFS(templates, "*.gomd") + tpl, err = tpl.ParseFS(tmplFS, "*.gomd")config/template.go (1)
55-61: Consider exposing Named metadata for Global/GroupGlobal and Group are typed as PropertySet. If templates may need section Name/Description, consider typing them as NamedPropertySet to expose that metadata without casting.
-type TemplateInfoData struct { +type TemplateInfoData struct { templates.DefaultData - Global, Group PropertySet + Global, Group NamedPropertySet Profile ProfileInfo KnownResticVersions []string }config/info_customizer.go (1)
85-103: Backup: note forbids “true” but type still allows bool – confirm intentYou remove the “true” example for non‑host in backup and add a note that “Boolean true is unsupported”, but info.mayBool remains true from earlier initialisation. If “true” must be rejected (and “false” allowed or not), consider tightening typing/validation; otherwise the schema still implies boolean is accepted.
Possible options:
- Documentation‑only (current): keep as is.
- Disallow booleans for non‑host in backup:
case constants.CommandBackup: if propertyName != constants.ParameterHost { - info.examples = info.examples[1:] + if len(info.examples) > 0 { + info.examples = info.examples[1:] // remove "true" + } + info.mayBool = false note = `Boolean true is unsupported in section "backup".`
- Or keep booleans but add an explicit validation note that only false is meaningful.
Also added a defensive len check around slicing to avoid a future panic if examples change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.golangci.bck.yml(1 hunks).golangci.yml(1 hunks).vscode/settings.json(1 hunks)Makefile(1 hunks)batt/battery.go(1 hunks)commands_display.go(1 hunks)commands_generate.go(1 hunks)complete_test.go(1 hunks)config/config.go(2 hunks)config/config_test.go(1 hunks)config/group.go(1 hunks)config/info.go(6 hunks)config/info_customizer.go(1 hunks)config/jsonschema/model.go(7 hunks)config/jsonschema/schema_test.go(2 hunks)config/profile.go(5 hunks)config/schedule.go(2 hunks)config/show_test.go(1 hunks)config/template.go(2 hunks)constants/global.go(0 hunks)constants/testing.go(1 hunks)crond/crontab_test.go(3 hunks)filesearch/filesearch.go(1 hunks)integration_test.go(1 hunks)lock/lock.go(2 hunks)lock/lock_test.go(4 hunks)main.go(1 hunks)main_test.go(4 hunks)monitor/hook/context.go(1 hunks)monitor/status/profile.go(1 hunks)own_command_error_test.go(1 hunks)own_commands.go(1 hunks)priority/priority.go(1 hunks)priority/prority_test.go(2 hunks)restic/commands.go(1 hunks)restic/downloader_test.go(0 hunks)restic/generator/main.go(4 hunks)schedule/handler_systemd.go(3 hunks)schedule/handler_windows.go(1 hunks)shell/command_test.go(4 hunks)systemd/generate.go(1 hunks)util/dotenv.go(1 hunks)
💤 Files with no reviewable changes (2)
- constants/global.go
- restic/downloader_test.go
🧰 Additional context used
🧬 Code graph analysis (9)
lock/lock_test.go (1)
constants/testing.go (1)
DefaultTestTimeout(6-6)
schedule/handler_systemd.go (1)
systemd/generate.go (1)
GetServiceFile(265-267)
priority/prority_test.go (1)
constants/testing.go (1)
DefaultTestTimeout(6-6)
config/info_customizer.go (3)
constants/command.go (1)
CommandBackup(5-5)constants/parameter.go (1)
ParameterHost(14-14)constants/section.go (1)
SectionConfigurationRetention(7-7)
shell/command_test.go (1)
constants/testing.go (1)
DefaultTestTimeout(6-6)
schedule/handler_windows.go (2)
schedule/permission.go (2)
PermissionUserBackground(12-12)PermissionUserLoggedOn(13-13)schtasks/permission.go (1)
UserAccount(17-17)
crond/crontab_test.go (4)
constants/testing.go (1)
DefaultTestTimeout(6-6)platform/darwin.go (1)
Executable(19-19)platform/other.go (1)
Executable(19-19)platform/windows.go (1)
Executable(21-21)
main.go (1)
priority/priority.go (1)
Values(16-23)
main_test.go (1)
constants/testing.go (1)
DefaultTestTimeout(6-6)
🔇 Additional comments (14)
schedule/handler_windows.go (1)
54-59: Behaviour preserved with the switch refactor.Branch coverage still aligns: both user permissions map correctly and anything else keeps the System default. Looks good.
util/dotenv.go (1)
72-74: Great fix for monotonic timestamp comparisonSwitching to
Time.Equalavoids false cache invalidations when the monotonic component differs, so this keeps the cache stable without compromising correctness. Nicely done.schedule/handler_systemd.go (3)
116-117: Nolint annotation keeps the silent exit path intentionalWelcome addition: the
//nolint:nilerrmakes the deliberate nil return clear to the linters and future readers.
145-150: Copy change aligns with new lint expectationsTweaked message punctuation still reads well and no behavioural change—looks good.
253-254: Explicit nolint documents the intentional swallowThanks for clarifying the ignored error; the
nolint:nilerrshould keep the new linters quiet.batt/battery.go (1)
81-85: Switch makes the state handling clearer.The refactor keeps the existing behaviour intact while making it easier to follow and extend the battery state checks. Nicely done.
complete_test.go (1)
114-114: Nice addition oft.Helper()Flagging this helper keeps failure reporting anchored to the real caller; good catch.
lock/lock_test.go (1)
31-45: Nice: build step now respects a test timeout.Using CommandContext with DefaultTestTimeout makes CI hangs far less likely.
commands_generate.go (1)
206-212: Section field addition looks good.Makes template data richer without breaking existing DefaultData usage.
config/config_test.go (1)
205-206: Tests correctly assert ContinueOnError across formats.Good coverage of unset/true/false cases with maybe.Bool.
Also applies to: 343-345
constants/testing.go (1)
5-7: Centralised test timeout works nicelyPulling the timeout into a shared constant keeps the suite consistent and makes future tweaks trivial.
config/template.go (1)
15-21: LGTM: TemplateData now exposes ProfileAdding Profile to TemplateData enables per‑profile templating. Usage in newTemplateData looks correct.
config/config.go (2)
250-268: Preallocating matchingUses is a good micro‑optimisationCapacity hint avoids reallocations on append. No behaviour change.
748-771: Direct Fprintf to builder reduces allocationsReplacing Sprintf+WriteString with Fprintf to the builder is cleaner and slightly more efficient.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
config/jsonschema/model.go (7)
71-101: Deduplication currently hinges on pointer identity; consider content-hash indexing to catch structurally identical single-use schemasRefs are introduced only when refs[item] > 1, where “item” is keyed by interface identity. Two structurally identical schemas instantiated separately and used once each won’t be deduplicated. Recommend maintaining a map[hash]string across the walk (hash from json.Marshal(item)) and:
- On first sighting of a hash, record its name (and store in $defs if above size threshold).
- On subsequent sightings of the same hash, replace with $ref regardless of pointer identity or local ref counts.
This will reduce schema size and improve reuse without relying on aliasing the same pointer instance.
Also applies to: 103-123
389-395: Suppress zero‑value noise in emitted schemas with omitemptyZero values are serialised today, producing minLength: 0, minItems: 0 and uniqueItems: false in every schema. Prefer omitting defaults to keep schemas concise.
Apply this diff:
- MinLength uint64 `json:"minLength"` + MinLength uint64 `json:"minLength,omitempty"`- MinItems uint64 `json:"minItems"` + MinItems uint64 `json:"minItems,omitempty"` - UniqueItems bool `json:"uniqueItems"` + UniqueItems bool `json:"uniqueItems,omitempty"`Also applies to: 323-326
337-344: Validate minItems/maxItems relationshipAdd a guard to prevent invalid ranges (minItems > maxItems) and only proceed to base verification when valid.
func (a *schemaArray) verify() (err error) { if a.Items == nil { err = fmt.Errorf("items of schemaArray is undefined") } else { - err = a.schemaTypeBase.verify() + if a.MaxItems != nil && a.MinItems > *a.MaxItems { + err = fmt.Errorf("minItems (%d) must be <= maxItems (%d)", a.MinItems, *a.MaxItems) + } + if err == nil { + err = a.schemaTypeBase.verify() + } } return }
397-406: Avoid overwriting earlier errors in schemaString.verifyA format error can overwrite a prior pattern error, obscuring the root cause. Guard the format check.
- if !slices.Contains(validFormatNames, s.Format) { + if err == nil && !slices.Contains(validFormatNames, s.Format) { err = fmt.Errorf("format %q is no valid string format", s.Format) }
346-354: Add numeric constraint checks for schemaNumber (multipleOf and bounds)Implement targeted validation for numeric constraints to catch invalid combinations early (and then delegate to base.verify).
Example addition (outside this hunk):
func (n *schemaNumber) verify() (err error) { if n.MultipleOf != nil && *n.MultipleOf <= 0 { return fmt.Errorf("multipleOf must be > 0") } // Inclusive bounds if n.Minimum != nil && n.Maximum != nil && *n.Minimum > *n.Maximum { return fmt.Errorf("minimum (%g) must be <= maximum (%g)", *n.Minimum, *n.Maximum) } // Interplay with exclusive bounds (2019‑09 uses numeric exclusives) if n.Minimum != nil && n.ExclusiveMaximum != nil && *n.Minimum >= *n.ExclusiveMaximum { return fmt.Errorf("minimum (%g) must be < exclusiveMaximum (%g)", *n.Minimum, *n.ExclusiveMaximum) } if n.ExclusiveMinimum != nil && n.Maximum != nil && *n.ExclusiveMinimum >= *n.Maximum { return fmt.Errorf("exclusiveMinimum (%g) must be < maximum (%g)", *n.ExclusiveMinimum, *n.Maximum) } if n.ExclusiveMinimum != nil && n.ExclusiveMaximum != nil && *n.ExclusiveMinimum >= *n.ExclusiveMaximum { return fmt.Errorf("exclusiveMinimum (%g) must be < exclusiveMaximum (%g)", *n.ExclusiveMinimum, *n.ExclusiveMaximum) } return n.schemaTypeBase.verify() }
160-176: Enforce uniqueness of type names when an array of types is usedJSON Schema requires the “type” array elements be unique. Add a quick duplicate check.
switch t := s.Type.(type) { case string: typeNames = []string{t} case []string: typeNames = t } if len(typeNames) == 0 { err = fmt.Errorf("expected single type or list of types, but none was specified") } + if len(typeNames) > 1 { + seen := make(map[string]struct{}, len(typeNames)) + for _, tn := range typeNames { + if _, ok := seen[tn]; ok { + return fmt.Errorf("duplicate type name %q in type list", tn) + } + seen[tn] = struct{}{} + } + } for _, name := range typeNames { if !slices.Contains(validTypeNames, name) { err = fmt.Errorf("type name %q not in (%s)", name, strings.Join(validTypeNames, ", ")) } }
421-425: Consider handling other unsupported lookarounds in verifySchemaRegexYou currently strip only negative lookahead (?!...). RE2 also doesn’t support (?=...), (?<=...), (?<!...). If these can appear in patterns, consider neutralising them similarly for verification or flagging them explicitly to improve diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks)config/jsonschema/model.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
.github/workflows/build.yml (1)
45-48: No action required: golangci-lint v2.5.0 tag exists upstream
The release tag v2.5.0 is available for golangci/golangci-lint.config/jsonschema/model.go (1)
214-220: oneOf/anyOf exposure and normalisation look goodClear split between anyOf/oneOf with verify-time normalisation of Title/Description is sound.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (66.67%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #562 +/- ##
==========================================
- Coverage 79.70% 79.69% -0.01%
==========================================
Files 137 137
Lines 13462 13467 +5
==========================================
+ Hits 10729 10732 +3
- Misses 2306 2310 +4
+ Partials 427 425 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Added more linter to the list, and made fixes accordingly