feat(git-sync): sync jobs debouncing [merge-ee-first] #7489
Conversation
Deploying windmill with
|
| Latest commit: |
ed3ebc5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d668be9a.windmill.pages.dev |
| Branch Preview URL: | https://git-sync-debouncing.windmill.pages.dev |
Signed-off-by: pyranota <[email protected]>
pyranota
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
|
Claude finished @pyranota's task in 2m 41s —— View job PR Review Complete
SummaryThis PR adds debouncing support for git-sync jobs ( Critical Issues
Issues to Address
Design Considerations
What Looks Good
Testing InstructionsTo verify the git-sync debouncing feature: Navigate to a workspace with git-sync configured (Workspace Settings → Git Sync). Deploy multiple scripts or flows in quick succession (e.g., via the UI or API). Observe that instead of creating individual sync jobs for each deployment, the system should debounce these into fewer sync operations. Check the Runs page filtered by "deployment_callback" jobs to verify the debouncing behavior - you should see consolidated commits like "[WM]: Deploy 15 objects" instead of 15 separate sync jobs. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub fn is_script_meets_min_version(&self, min_version: u32) -> error::Result<bool> { | ||
| // example: "hub/28102/sync-script-to-git-repo-windmill" | ||
| let current: u32 = self | ||
| .script_path | ||
| .split("/") // -> ["hub" "28102" "sync-script-to-git-repo-windmill"] | ||
| .skip(1) // emit "hub" | ||
| .next() // get numeric id | ||
| .ok_or(Error::InternalErr(format!( | ||
| "cannot get script version id from: {}", | ||
| &self.script_path | ||
| ))) | ||
| .and_then(|s| { | ||
| s.parse().map_err(|e| { | ||
| Error::InternalErr(format!( | ||
| "cannot get script version id from: {}. e: {e}", | ||
| &self.script_path | ||
| )) | ||
| }) | ||
| })?; | ||
|
|
||
| Ok(current >= min_version) // this works on assumption that all scripts in hub have sequential ids | ||
| } |
There was a problem hiding this comment.
Concern: Assumption about sequential hub script IDs
The comment states this "works on assumption that all scripts in hub have sequential ids". This assumption may be fragile:
- Hub scripts could be deleted - IDs might not be strictly sequential
- Different hub scripts - Not all hub scripts are the sync script; comparing IDs across different scripts doesn't indicate version
- Non-hub paths - If
script_pathdoesn't follow thehub/{id}/{name}format, this will fail
Consider adding more robust error handling or documentation about when this method should be called and what guarantees exist about hub script ID ordering.
Signed-off-by: pyranota <[email protected]>
…ll into git-sync-debouncing
Signed-off-by: pyranota <[email protected]>
Signed-off-by: pyranota <[email protected]>
This commit updates the EE repository reference after PR #371 was merged in windmill-ee-private. Previous ee-repo-ref: 346d38e91776e987f88866543adbf78a9c721073 New ee-repo-ref: bc904859dd66c55ebad002e8526103c73de841cd Automated by sync-ee-ref workflow.
|
🤖 Updated |
Drastically improve sync jobs performance.