Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow for empty files on hook check #3117

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

nzspambot
Copy link
Contributor

@nzspambot nzspambot commented Dec 3, 2024

Description

Empty hook will fail on agent on run

Context

We produce an empty checkout hook file :

checkout_override_file=$(mktemp "${rw_root_dir}/tmp/checkout-override.XXXXXXXX")
    # We need to have a custom override for the checkout hook. This copies an
    # empty checkout file so buildkite-agents can still run the pre-checkout and post-checkout
    # hooks. This is very much necessary as a several plugins seem to rely on these
    # hooks to run their own logic like secrets-manager
    # We're also copying the git repo structure as well. This preserves
    # the current file structure, mainly due to developers using
    # this exact path for their pipeline scripts. Without this file path structure
    # pipelines end up breaking.

This currently doesn't work as the file is empty as the hook check will fail (this is your extracted function):

bash-5.2$ go run /tmp/main.go -file /tmp/checkout-override.kFBBE4Ia 
Error: reading first four bytes of file "/tmp/checkout-override.kFBBE4Ia": EOF

image

Changes

Adds a check to see if the file has 4 bytes if not then return nil (this is your updated function):

bash-5.2$ go run /tmp/main.go -file /tmp/checkout-override.kFBBE4Ia 
The file /tmp/checkout-override.kFBBE4Ia is not a binary executable.

Testing

I would like to test this but see no tests around this? unless I am missing them, also it builds fine locally

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nzspambot! Looking good, I have one comment.

Re the lack of tests, I agree it would be good to have a couple of fixtures to test this function. I might add something. I don't see it as a blocker to this PR though.

internal/job/hook/binary.go Outdated Show resolved Hide resolved
@nzspambot
Copy link
Contributor Author

thanks for the review @DrJosh9000

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nzspambot
Copy link
Contributor Author

thanks @DrJosh9000 I think you'll need to merge as that status check is missing 👍🏻

@DrJosh9000 DrJosh9000 merged commit e948232 into buildkite:main Dec 4, 2024
1 check passed
@nzspambot nzspambot deleted the jbaker/fix-hook-check branch December 4, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants