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

[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]

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