[Feat] Pre exechook by Mohamed-elg · Pull Request #990 · kubernetes/git-sync

Mohamed-elg · 2026-06-11T20:21:55Z

/kind feature

What this PR does:
Fixes #937
adds flags for --pre-exechook-command, --pre-exechook-timeout and --pre-exechook-backoff to run a command similarly to the exechook but before the sync

Follows #937 (comment)

linux-foundation-easycla · 2026-06-11T20:22:03Z

The committers listed above are authorized under a signed CLA.

✅ login: Mohamed-elg / name: Mohamed-elg (757c143)

k8s-ci-robot · 2026-06-11T20:22:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Mohamed-elg
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2026-06-11T20:22:04Z

Welcome @Mohamed-elg!

It looks like this is your first PR to kubernetes/git-sync 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/git-sync has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

thockin

Thanks for the PR! Notes below. Also, this will need e2e tests

thockin · 2026-06-12T17:02:34Z

            The timeout for the --exechook-command.  If not specifid, this
            defaults to 30 seconds ("30s").

+    --pre-exechook-backoff <duration>, $GITSYNC_PRE_EXECHOOK_BACKOFF


Flags should be in alphabetic-sorted order, sorry

thockin · 2026-06-12T17:50:04Z

+            not specified, this defaults to 3 seconds ("3s").
+
+    --pre-exechook-command <string>, $GITSYNC_PRE_EXECHOOK_COMMAND
+            An optional command to be executed before syncing a new hash of the


This isn't quite right.

I think we should update the docs on --exechook-command to say something like:

"""
An optional command to be executed after syncing a new hash of the remote repository and publishing the symlink (see --link).
"""

Then this can be:

"""
An optional command to be executed after syncing a new hash of the remote repository but before publishing the symlink (see --link).
"""

The rest should the same. What do you think?

you are right, i changed the logic to have the hooks around the symlink rather than the sync event itself

thockin · 2026-06-12T18:04:21Z

 		start := time.Now()
 		ctx, cancel := context.WithTimeout(context.Background(), *flSyncTimeout)

+		if preExechookRunner != nil {


This doesn't seem right? This will send the old hash on every loop, then sync a new hash. Unless I totally misunderstood the request, you want to try to sync, and ONLY IF there is a new hash, call the hook before publishing the symlink.

Did I misunderstand?

Unfortunately, the sync and the publish are inside git.SyncRepo. You can either decompose that (which would need to share a bunch of state) or you could pass function pointers into it like we do for refreshCreds - pass a struct like:

type syncHooks struct { refreshCreds func(context.Context) error beforePublish func(context.Context, changed bool, hash string) error afterPublish func(context.Context, changed bool, hash string) error }

correct, fixed it by moving the logic inside git.SyncRepo as suggested

Mohamed-elg · 2026-06-15T12:42:43Z

Thanks for the PR! Notes below. Also, this will need e2e tests

will add the e2e tests once the changes are validated

thockin

Thanks! My comments are pretty superficial. I think the logic looks good. e2e tests will prove it.

thockin · 2026-07-01T23:44:20Z

 	env = append(env, envKV("GITSYNC_HASH", hash))

-	h.log.V(0).Info("running exechook", "hash", hash, "command", h.command, "timeout", h.timeout)
+	h.log.V(0).Info("running hook", "name", h.name, "hash", hash, "command", h.command, "timeout", h.timeout)


super nitpicky, but I would leave the key as "exechook". If this is the only thing I flag, I will overlook it :)

thockin · 2026-07-01T23:48:29Z

 		envDuration(3*time.Second, "GITSYNC_EXECHOOK_BACKOFF", "GIT_SYNC_EXECHOOK_BACKOFF"),
 		"the time to wait before retrying a failed exechook")

+	flPreExechookCommand := pflag.String("pre-exechook-command",


I am struggling with the name -- "pre-exechook" makes it sound like it runs before the exechook.

How about "prepub-exechook-*" ? Any better ideas?

thockin · 2026-07-01T23:49:15Z

 		"the time to wait before retrying a failed exechook")

+	flPreExechookCommand := pflag.String("pre-exechook-command",
+		envString("", "GITSYNC_PRE_EXECHOOK_COMMAND", "GIT_SYNC_PRE_EXECHOOK_COMMAND"),


We don't need the old-style names "GIT_SYNC_..." - those are just for backwards compat in older flags.

thockin · 2026-07-01T23:51:15Z

            An optional command to be executed after syncing a new hash of the
-            remote repository.  This command does not take any arguments and
+            remote repository and publishing the symlink (see --link).
+            This command does not take any arguments and


Please reflow this to 80 columns (it looks like "executes" can fold back up a line) but make sure to use spaces and not tabs. Sorry, this is annoying, but I like the extra detail

thockin · 2026-07-01T23:52:03Z

+            remote repository but before publishing the symlink (see --link).
+            This command does not take any arguments and
+            executes with the synced repo as its working directory. The
+            $GITSYNC_HASH environment variable will be set to the previous git hash that


please reflow this whole block to 80 columns

thockin · 2026-07-02T00:15:12Z

@@ -1842,7 +1908,12 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con

 		// If we have a new hash, update the symlink to point to the new worktree.
 		if changed {


This is super subtle, and we should leave a comment - I had to walk through it to understand the subtlety.

Something like:

"""
If the previous run crashed before publishing the link, then we must call the pre-publish hook, and since changed is true, we will. If the previous run crashed after publishing the link, then we do not need to call the pre-publish hook, and since changed is false, we won't. The post-publish hooks are called in both cases.

pre hook

757c143

k8s-ci-robot requested review from stp-ip and thockin June 11, 2026 20:22

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2026

Mohamed-elg marked this pull request as draft June 11, 2026 20:23

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 11, 2026

Mohamed-elg added 2 commits June 11, 2026 22:38

readme + tab fix

2d84026

fix def

47f28c6

Mohamed-elg marked this pull request as ready for review June 11, 2026 20:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2026

k8s-ci-robot requested review from janetkuo and sdowell June 11, 2026 20:56

thockin reviewed Jun 12, 2026

View reviewed changes

change logic around symlink

9812353

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 15, 2026

Mohamed-elg requested a review from thockin June 29, 2026 11:57

thockin reviewed Jul 2, 2026

View reviewed changes

		@@ -1842,7 +1908,12 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con

		// If we have a new hash, update the symlink to point to the new worktree.
		if changed {

Uh oh!

Conversation

Mohamed-elg commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 11, 2026

Uh oh!

k8s-ci-robot commented Jun 11, 2026

Uh oh!

thockin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed-elg commented Jun 15, 2026

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mohamed-elg commented Jun 11, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Jun 11, 2026 •

edited

Loading

thockin left a comment •

edited

Loading