Add support for configurable Plan Preview behavior in piped

[rawadhossain]

What this PR does:
Adds a configurable planPreview section to piped config (PipedSpec in both pkg/config and pkg/configv1).

These values are validated and then passed to planpreview.NewHandler using existing options:

• WithWorkerNum
• WithCommandQueueBufferSize
• WithCommandCheckInterval
• WithCommandHandleTimeout

The configuration is applied in both piped entrypoints (pkg/app/piped/cmd/piped and pkg/app/pipedv1/cmd/piped).

Also includes tests to ensure:

• YAML parsing works correctly
• Invalid (negative) values fail validation

Why we need it:
Currently plan preview behavior can only be tuned internally (mainly in tests).
This change allows to configure things like worker count, queue size, and timing directly from piped config.

Fixes Issue #5916

Does this PR introduce a user-facing change?:

• How are users affected by this change:
  • Users can optionally add a spec.planPreview block in piped YAML:
    • workerNum
    • commandQueueBufferSize
    • commandCheckInterval
    • commandHandleTimeout (duration format like 5s, 10m)
  • If not provided, existing default behavior remains unchanged
• Is this breaking change: No
• How to migrate (if breaking change): N/A

[armistcxy]

Hi @rawadhossain, let me help you review this PR ^^

[rawadhossain]

Sure @armistcxy, please take a look.

[armistcxy]

CommandCheckInterval will later be used for creating time.Ticker

commandTicker := time.NewTicker(h.options.commandCheckInterval)

This will cause panic if h.options.commandCheckInterval = 0

[rawadhossain]

right, time.NewTicker will panic for non positive durations, so this definitely needs to be guarded.

We handle this the same way as above in piped.go by passing the option when the value is > 0, so zero isn’t forwarded and the handler uses its default interval.

if cfg.PlanPreview.CommandCheckInterval > 0 {
    ppOpts = append(ppOpts, planpreview.WithCommandCheckInterval(cfg.PlanPreview.CommandCheckInterval.Duration()))
}

[armistcxy]

same reason with v0, commandCheckInterval = 0 would cause panic

[armistcxy]

IMO, this is dangerous, I would think a safer approach is checking whether these values from PlanPreview options are not zero value first: if it != 0, then add the option

For example

if cfg.PlanPreview.WorkerNum > 0 {
    planpreview.WithWorkerNum(cfg.PlanPreview.WorkerNum),
}

[rawadhossain]

ahh, good one. Yeah, how about I frame it like this, does it seem right now:

ppOpts := []planpreview.Option{
			planpreview.WithLogger(input.Logger),
		}
		if cfg.PlanPreview.WorkerNum > 0 {
			ppOpts = append(ppOpts, planpreview.WithWorkerNum(cfg.PlanPreview.WorkerNum))
		}
		if cfg.PlanPreview.CommandQueueBufferSize > 0 {
			ppOpts = append(ppOpts, planpreview.WithCommandQueueBufferSize(cfg.PlanPreview.CommandQueueBufferSize))
		}
		if cfg.PlanPreview.CommandCheckInterval > 0 {
			ppOpts = append(ppOpts, planpreview.WithCommandCheckInterval(cfg.PlanPreview.CommandCheckInterval.Duration()))
		}
		if cfg.PlanPreview.CommandHandleTimeout > 0 {
			ppOpts = append(ppOpts, planpreview.WithCommandHandleTimeout(cfg.PlanPreview.CommandHandleTimeout.Duration()))
		}

h := planpreview.NewHandler(
			//....
			ppOpts...,
		)

[armistcxy]

it seems like a good approach now, i think you should verify whether CommandQueueBufferSize = 0 is valid. Go has buffer size = 0 for channel right ^ ^, maybe this case can be similar

[rawadhossain]

Yeah I checked this, since we do make(chan ..., commandQueueBufferSize) in the handler, make(chan T, 0) is valid in Go (unbuffered), so CommandQueueBufferSize = 0 itself wouldn’t break anything.

in pkg/app/piped/planpreview/handler.go:

h := &Handler{
		gitClient:     gc,
		commandLister: cl,
		commandCh:     make(chan model.ReportableCommand, opt.commandQueueBufferSize),
		prevCommands:  map[string]struct{}{},
		options:       opt,
		logger:        opt.logger.Named("plan-preview-handler"),
	}

The tricky part is config: since it’s a plain int, omitted and 0 both become 0 after decode. So if we pass 0 through, even “not set” would behave like unbuffered instead of using the default (10), which might be unexpected.

Right now with the > 0 guard, both cases just fall back to the default, which I felt safer.

If we want to support unbuffered explicitly, we’d probably need a way to distinguish omitted vs 0.

What do you think, should we support that, or keep default-only for now?

[armistcxy]

Same reason as v0

[armistcxy]

You allow WorkerNum = 0, I scare that commands can be block by this allowance, please verify the behavior to find the right condition for validating

[rawadhossain]

Good point, I checked the handler behavior here in piped/planpreview/handler.go:

for i := 0; i < h.options.workerNum; i++ {
		go startWorker(ctx, h.commandCh)
	}

From the handler logic, workerNum = 0 would result in no workers being started, so commands wouldn’t be processed if it reached there as is.

In the current flow, we guard this at the callsite (> 0 check in piped.go), so a zero value isn’t passed to WithWorkerNum and the handler falls back to its default worker count.

I’m happy to enforce > 0 in Validate() as well if you think that’s preferable, though it may also affect cases where the field is omitted and defaults are expected.

[armistcxy]

I'm not sure the right thing to do here, but isn't that we are using defensive programming in a lot of place, I don't think this is a good idea

maybe just concentrate on validating config at one place, what do you think ?

[rawadhossain]

yeah, I think that would be cleaner. I’ve moved the > 0 checks into the With* option functions in the handler, so the “0 = keep default” logic is handled in one place now.

Validate() is just handling invalid cases (like negatives). Since with plain int/Duration, omitted and 0 both decode the same after YAML parsing, we can’t really reject 0 there without breaking configs where users just omit the planPreview block.

Let me know your thoughts on this.

[rawadhossain]

@armistcxy heya I responded to your comments, PTAL.
Will commit the changes if this seems right.

[rawadhossain]

hey @armistcxy sorry couldn't respond earlier, was caught up with exams. I made some changes and replied to your follow-ups. PTAL.