GIT_SYNC_EXECHOOK_COMMAND + GIT_SYNC_ONE_TIME

[ChrisERo]

Is a command specified by the EXECHOOK_COMMAND environment variable always supposed to execute after git sync pulls/clones a git repo? The documentation suggests this, but when GIT_SYNC_ONE_TIME is true, I do not see where this is enforced. It seems possible that the main thread could execute os.Exit before the exechook/consumer thread processes the "sync event" and executes the specified command.

I looked at version 3.3.4 and it looks like it does not have such a race condition, as the exechook is run by the same main thread.

Forgive me if this is not the correct venue to post this question/issue; it's my first "contribution" to an open-source project.

[thockin]

Good question. That intersection is not well defined. Is it useful to make such a guarantee? If so, we need a way to signal back from the exec and webhook runners that they are done. Or maybe signal "stop" to the runners and they signal "done" back. On the one-time path, before exit, stop the runners. The alternative would be to make those flags mutually-exclusive, which is a breaking change, so at best we would just warn and not set up the runners. PRs or simpler ideas welcome!

[ChrisERo]

Hi @thockin, thanks for the prompt response!!

I would find this guarantee to be useful. In my personal use-case, I only need git-sync to perform a one-time 'git clone' on a git tag and store the source code in a specific, preexisting folder. Ideally, git-sync would be an init-container in this scenario, explaining my dependence on GIT_SYNC_ONE_TIME. To put the sourcecode in the right location, I am relying on a shell script inside git-sync container's volume, referenced by GIT_SYNC_EXECHOOK_COMMAND.

I would be happy to take a crack at drafting this PR. I see that in the previous release, the sync-hook was executed in the same thread that terminates if the "ONE_TIME" flag is true. Is this an acceptable solution: removing the asynchrony involved with exec/sync hook?

[thockin]

If we make this guarantee for exec, we should make it for webhook, too. But I don't think we should serialize those, so the simple solution you propose isn't really ideal.

…

On Thu, Jan 6, 2022, 7:34 PM ChrisERo ***@***.***> wrote: Hi @thockin <https://github.com/thockin>, thanks for the prompt response!! I would find this guarantee to be useful. In my personal use-case, I only need git-sync to perform a one-time 'git clone' on a git tag and store the source code in a specific, preexisting folder. Ideally, git-sync would be an init-container in this scenario, explaining my dependence on *GIT_SYNC_ONE_TIME*. To put the sourcecode in the right location, I am relying on a shell script inside git-sync container's volume, referenced by *GIT_SYNC_EXECHOOK_COMMAND*. I would be happy to take a crack at drafting this PR. I see that in the previous release <https://github.com/kubernetes/git-sync/blob/v3.3.4/cmd/git-sync/main.go#L831>, the sync-hook was executed in the same thread that terminates if the "ONE_TIME" flag is true. Is this an acceptable solution: removing the asynchrony involved with exec/sync hook? — Reply to this email directly, view it on GitHub <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVFW3455F2EOEZSJ7KLUUZNLNANCNFSM5LLVFG2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[ChrisERo]

Thanks for the insight!

I created the following PR, #466, the code hasn't been tested, but I want to make sure this is the general strategy you were looking for before proceeding.

[ChrisERo]

Hi @thockin, just updated the PR to address your concerns, except the e2e tests. I added tests for exechook, but want to make sure that the pattern they use suffices before using it to test webhook.