fix(cmd): negative or zero parallelism#2114
Conversation
The --parallelism flag was passed without validation. A value < 1 would hang or panic: Solve blocks forever on a nil event channel, and the validator either hangs or panics on a negative channel buffer. Added a checkParallelism helper and called it early with a clear usage error.
Adding unit tests for the parallelism fix
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2114 +/- ##
==========================================
+ Coverage 33.17% 34.12% +0.94%
==========================================
Files 78 78
Lines 7433 7443 +10
==========================================
+ Hits 2466 2540 +74
+ Misses 4763 4669 -94
- Partials 204 234 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
manishsvk
changed the title
manishsvk
changed the title
|
LGTM - will approve once CI is fixed |
manishsvk
changed the title
|
This should have a go.mod update with a new GDR version that has the changes added here: |
We skipped it because it was not strictly required - it can go with next gdr release IMO |
harshadixit12
dismissed
their stale review
let's do the GDR version bump as well
| module github.com/kong/deck | ||
|
|
||
| go 1.26.3 | ||
| go 1.26.4 |
There was a problem hiding this comment.
@manishsvk since the golang version is changing, let's update the docker file as well to build with this version?
The digest for new version can be copied from https://hub.docker.com/layers/library/golang/1.26.4/images/sha256-84e4dd2658fb85e1debf9fbe653aa386ed5bf108429ea641ab0a7db31a8ed5cc
There was a problem hiding this comment.
We can do this here for onboarding work.
For future references, such updates are not necessary. Dockerfile picks up the go version from go.mod. The version in dockerfile is a "fallback". Ref
| func syncMain(ctx context.Context, filenames []string, dry bool, parallelism, | ||
| delay int, workspace string, enableJSONOutput bool, applyType ApplyType, | ||
| ) error { | ||
| if err := checkParallelism(parallelism); err != nil { |
There was a problem hiding this comment.
This check should be in PreRunE hook for the diff, sync, apply commands.
There was a problem hiding this comment.
IMO having it here avoids duplication, any reason why we want in PreRunE hook instead?
There was a problem hiding this comment.
It does avoid duplication but PreRunE is the hook for such things to fail early. We have other checks in syncMain because they rely on targetState building.
It's mostly about Separation of Concerns. A validity check on a flag or argument goes in PreRunE.
5 participants
Summary
This change adds a
checkParallelismhelper so an invalid value is rejected up front with a usage error, before any state file is read or any request is sent to Kong. Valid values (>= 1) are unaffected.Full changelog
checkParallelismhelper incmd/common.go]--parallelism < 1(covers gatewaysync,diff,applyandvalidatecommands)]go-database-reconcilerversion]checkParallelism(zero, negative, positive)]Issues resolved
Fix #375
Testing