Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates bgpipe’s documentation and navigation, bumps the bgpfix dependency, and introduces a configurable stage stop timeout along with lifecycle tests to validate stage start/stop behavior.
Changes:
- Bump
github.com/bgpfix/bgpfixtov0.17.1and adaptstages/grep.goto the updated eval/context API. - Add
StageOptions.StopTimeoutand update stop logic to optionally force-cancel stages after a configurable timeout; add lifecycle tests. - Refresh docs/site structure (mkdocs nav, rewritten intro/index/filter/json docs) and add a new Flowspec JSON format reference.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| stages/grep.go | Switch filter evaluation setup to a single eval.Set(...) call using pipe.UseContext. |
| core/stage.go | Add StopTimeout time.Duration to stage options. |
| core/run.go | Respect StopTimeout when stopping stages (wait up to timeout before force-cancel). |
| core/lifecycle_test.go | Add stage lifecycle and coordination tests (build-tagged !race). |
| go.mod / go.sum | Bump bgpfix to v0.17.1; tweak commented replace line. |
| mkdocs.yml | Restructure navigation; rename site tagline. |
| docs/* | Major documentation refresh; add docs/flowspec.md; update quickstart and stage docs. |
| README.md | Update project description/links and author year. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Bidir bool // allow -LR (bidir mode)? | ||
| FilterIn bool // allow stage input filtering? (must have callbacks) | ||
| FilterOut bool // allow stage output filtering? (must have inputs) | ||
| StopTimeout time.Duration // timeout for Run to exit after Stop (default 3s, zero = default) |
There was a problem hiding this comment.
StopTimeout has a special-case behavior in runStop for negative durations (t < 0 cancels immediately), but the field comment only documents zero vs default. Please document what negative values mean (or avoid using negative as a sentinel) so stage authors don’t accidentally trigger immediate cancellation.
| StopTimeout time.Duration // timeout for Run to exit after Stop (default 3s, zero = default) | |
| StopTimeout time.Duration // timeout for Run to exit after Stop (default 3s; zero = default; negative = cancel immediately) |
| // wait for stage to enter Run | ||
| time.Sleep(50 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
Several tests rely on fixed time.Sleep(...) delays to wait for stages to enter Run(). This is prone to flakes under load/slow CI. Prefer synchronizing on a channel/event (e.g., signal from runFn) or polling with a deadline instead of sleeping a fixed duration.
| if elapsed < 80*time.Millisecond { | ||
| t.Fatalf("runStop returned too quickly (%v), expected ~100ms timeout", elapsed) | ||
| } |
There was a problem hiding this comment.
The timeout assertion is quite tight (expects ~100ms, failing if <80ms). On fast machines, scheduler granularity or timing jitter can make this non-deterministic. Consider asserting the behavior (that cancellation happens) with a generous upper bound, or measure using synchronization (e.g., wait for Ctx.Done()/done plus a max duration) rather than elapsed wall-clock thresholds.
| if elapsed < 80*time.Millisecond { | |
| t.Fatalf("runStop returned too quickly (%v), expected ~100ms timeout", elapsed) | |
| } |
| Kill the session if a default route is announced: | ||
|
|
||
| ```bash | ||
| bgpipe \ | ||
| -- connect 192.0.2.1 \ | ||
| -- grep --keep --kill-match --event-match default 'prefix == 0.0.0.0/0' \ | ||
| -- grep --kill-match default 'prefix == 0.0.0.0/0' \ | ||
| -- connect 10.0.0.1 |
There was a problem hiding this comment.
The example command uses --kill-match default, but --kill-match is a boolean flag in the implementation. This makes default get parsed as the positional FILTER argument (and the actual filter becomes an extra arg), so the example won’t work as written. Also, this option kills the process (per the option description), not just the session—if the goal is to stop a session/pipeline on match, document the correct event/stop-based approach instead.
No description provided.