Conversation
Add GitHub Actions workflows for build, lint, test, security scanning (CodeQL, Codacy, gitleaks, govulncheck, gosec), SBOM generation, and SLSA provenance. Add Dependabot config for automated dependency updates. Add Makefile with targets for test, lint, vet, sec, and toolchain management. Add .golangci.yaml with comprehensive golangci-lint v2 configuration and .project-settings.env for shared tool version pinning. Refactor cron scheduler: - Replace sort.Sort(byTime) with slices.SortFunc and explicit comparators - Extract schedulerLoop() into focused helper methods (initializeEntries, processSchedulerEvent, handleTimerFired, handleEntryAdded, etc.) - Use sync.WaitGroup.Go in startJob instead of manual Add/Done - Wrap parse errors with context in AddJob via fmt.Errorf %w - Add errPanicValue sentinel and use %w wrapping in Recover() - Replace deprecated io/ioutil with io throughout tests Improve test quality: - Add t.Parallel() to all tests and subtests - Extract magic numbers and strings into named constants - Add test helpers (mustAddFunc, mustAddJob, requireContextDoneWithin, requireContextPendingFor, waitForJobStarted, signalJobStarted) - Refactor TestStopAndWait and TestChainSkipIfStillRunning subtests into separate top-level functions for clarity - Update doc.go and README.md to modern Go doc-comment format
BREAKING CHANGE: This is a full API modernization for the v4 release. - Rename module to github.com/hyp3rd/cron/v4 - Job.Run now takes context.Context and returns error - Start(ctx), Run(ctx), Stop(ctx) all accept context.Context - Replace custom Logger interface with *slog.Logger - Rename ScheduleParser → Parser, Parser → SpecParser - Introduce Clock/Timer interfaces with WithClock option - Rewrite tests to use fakeClock for deterministic scheduling - Add CHANGELOG.md and MIGRATION.md - Rewrite README.md and doc.go for v4 API - Delete .travis.yml (CI is GitHub Actions)
There was a problem hiding this comment.
Pull request overview
This PR modernizes the cron library for a v4 release by updating the public API to be context-aware, switching logging to log/slog, and adding a pluggable Clock to enable deterministic scheduling tests. It also updates module path/versioning, documentation, and CI tooling to reflect the new major version.
Changes:
- Introduces a breaking scheduler API (
context.Context+error-returning jobs, context-driven Start/Run/Stop). - Reworks parsing/scheduling internals and rewrites tests around a deterministic fake clock.
- Updates docs, migration/changelog, module path, and CI/lint/security workflows.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| spec.go | Refactors SpecSchedule.Next and bounds handling. |
| spec_test.go | Restructures schedule tests into parallel subtests and shared helpers/constants. |
| parser.go | Renames/modernizes parser types and refactors parsing/validation logic. |
| parser_test.go | Updates parser tests for new parser naming and helpers/constants. |
| cron.go | Implements the new v4 scheduler lifecycle (context, slog, Clock) and internal loop refactor. |
| cron_test.go | Large test rewrite to use fake clock and new context-based APIs. |
| chain.go | Updates wrappers for context/error semantics and slog; introduces ErrPanic. |
| chain_test.go | Updates wrapper tests for new context/error behavior. |
| constantdelay.go | Minor formatting/behavior-preserving tweak. |
| constantdelay_test.go | Test refactor with shared constants and minor cleanup. |
| clock.go | Adds Clock/Timer abstractions and system clock implementation. |
| clock_fake_test.go | Adds deterministic fake clock implementation for tests. |
| option.go | Updates options to use *slog.Logger, new parser types, and adds WithClock. |
| option_test.go | Updates option tests to validate new logger and clock behavior. |
| helpers_test.go | Adds shared test helpers (parser, add-job helpers, location loader, type assertions). |
| logger.go | Replaces custom logger interface with *slog.Logger defaults. |
| README.md | Rewrites README for v4 usage, features, and installation. |
| doc.go | Updates package docs/examples for v4 API and logging. |
| MIGRATION.md | Adds a v3→v4 migration guide covering breaking changes. |
| CHANGELOG.md | Adds a v4 changelog entry describing the release changes. |
| go.mod | Updates module path to github.com/hyp3rd/cron/v4 and Go version directive. |
| LICENSE | Updates license header content. |
| Makefile | Adds project Makefile targets for testing/linting/security tooling. |
| .project-settings.env | Adds shared project settings used by Makefile/workflows. |
| .golangci.yaml | Adds golangci-lint configuration. |
| .gitignore | Adds common local/editor ignores. |
| .travis.yml | Removes legacy Travis CI config. |
| .github/workflows/test.yml | Adds Go test workflow with caching and coverage artifact. |
| .github/workflows/go.yml | Adds Go build/verify workflow. |
| .github/workflows/lint.yml | Adds lint workflow (gci/gofumpt/staticcheck/golangci-lint). |
| .github/workflows/security.yml | Adds scheduled/PR security checks (govulncheck, gosec). |
| .github/workflows/sbom.yml | Adds scheduled SBOM generation workflow. |
| .github/workflows/provenance.yml | Adds release provenance/signing workflow. |
| .github/workflows/gitleaks.yml | Adds secret scanning workflow via gitleaks. |
| .github/workflows/codeql.yml | Adds CodeQL analysis workflow. |
| .github/workflows/codacy.yml | Adds Codacy security scan workflow. |
| .github/dependabot.yml | Enables Dependabot updates for Go modules and GitHub Actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c.jobWaiter.Go(func() { | ||
| err := j.Run(ctx) | ||
| if err != nil { | ||
| c.logger.Warn("job error", "err", err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
sync.WaitGroup does not provide a Go method (in current Go versions), so c.jobWaiter.Go(...) will not compile and jobs won’t run. Use the usual Add(1) + goroutine + Done pattern (or switch to an errgroup-like type) to track in-flight jobs.
| c.jobWaiter.Go(func() { | |
| err := j.Run(ctx) | |
| if err != nil { | |
| c.logger.Warn("job error", "err", err) | |
| } | |
| }) | |
| c.jobWaiter.Add(1) | |
| go func() { | |
| defer c.jobWaiter.Done() | |
| err := j.Run(ctx) | |
| if err != nil { | |
| c.logger.Warn("job error", "err", err) | |
| } | |
| }() |
| func extractLocation(spec string) (string, *time.Location, error) { | ||
| loc := time.Local | ||
|
|
||
| if !strings.HasPrefix(spec, "TZ=") && !strings.HasPrefix(spec, "CRON_TZ=") { | ||
| return spec, loc, nil | ||
| } | ||
|
|
||
| i := strings.Index(spec, " ") | ||
| eq := strings.Index(spec, "=") | ||
|
|
||
| loc, err := time.LoadLocation(spec[eq+1 : i]) | ||
| if err != nil { | ||
| return "", nil, fmt.Errorf("provided bad location %s: %w", spec[eq+1:i], err) | ||
| } | ||
|
|
||
| return strings.TrimSpace(spec[i:]), loc, nil |
There was a problem hiding this comment.
extractLocation can panic on malformed specs like "TZ=UTC" (missing trailing space/fields) or missing '=' because strings.Index returns -1 and the subsequent slices use negative indexes. Since specs are user input, this should return a validation error instead of panicking; guard i/eq and ensure the prefix token contains both '=' and a following space.
| func monthBounds() bounds { | ||
| return bounds{minMonthValue, maxMonthValue, map[string]uint{ | ||
| "jan": monthJanuaryValue, | ||
| "feb": monthFebruaryValue, | ||
| "mar": monthMarchValue, | ||
| "apr": monthAprilValue, | ||
| "may": monthMayValue, | ||
| "jun": monthJuneValue, | ||
| "jul": monthJulyValue, | ||
| "aug": monthAugustValue, | ||
| "sep": monthSeptemberValue, | ||
| "oct": monthOctoberValue, | ||
| "nov": monthNovemberValue, | ||
| "dec": monthDecemberValue, | ||
| }} | ||
| } | ||
|
|
||
| func dayOfWeekBounds() bounds { | ||
| return bounds{minDayOfWeekValue, maxDayOfWeekValue, map[string]uint{ | ||
| "sun": weekdaySundayValue, | ||
| "mon": weekdayMondayValue, | ||
| "tue": weekdayTuesdayValue, | ||
| "wed": weekdayWednesdayValue, | ||
| "thu": weekdayThursdayValue, | ||
| "fri": weekdayFridayValue, | ||
| "sat": weekdaySaturdayValue, | ||
| }} | ||
| } |
There was a problem hiding this comment.
monthBounds() and dayOfWeekBounds() allocate a new map on every call. These functions are invoked during parsing (e.g., via parseScheduleFields), so this introduces avoidable allocations per Parse(). Consider keeping the name maps as package-level variables (or returning shared immutable maps) so bounds construction is allocation-free.
| // Advance moves the clock forward by dur and fires all timers whose deadlines | ||
| // have been reached. Timers fire in deadline order. | ||
| func (fc *fakeClock) Advance(dur time.Duration) { | ||
| fc.mu.Lock() | ||
| defer fc.mu.Unlock() | ||
|
|
||
| fc.cur = fc.cur.Add(dur) | ||
| fc.fireTimersLocked() | ||
| } |
There was a problem hiding this comment.
The comment says "Timers fire in deadline order", but fireTimersLocked iterates fc.timers in insertion order without sorting by deadline. Either sort timers by deadline before firing, or adjust the comment so it matches the actual behavior.
| Copyright (C) 2026 Francesco Cosentino | ||
| All Rights Reserved. | ||
|
|
||
| MIT LICENSE |
There was a problem hiding this comment.
The LICENSE header replaces the original copyright notice entirely. For MIT-licensed forks, you generally need to preserve the upstream copyright notice(s) and add your own (not replace them), otherwise it can violate the license’s attribution requirement. Consider restoring the original notice and appending your copyright line instead.
| all: .PHONY | ||
|
|
There was a problem hiding this comment.
all: .PHONY is not a correct way to declare a phony target, and it makes all depend on a (likely non-existent) file named .PHONY. If you intend all to be phony, declare it in the .PHONY: list and (optionally) make it depend on other targets (e.g., all: test lint).
BREAKING CHANGE: This is a full API modernization for the v4 release.