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

Add nonzero exit behavior to hook documentation #2817

Closed
wants to merge 4 commits into from

Conversation

thejcannon
Copy link
Contributor

From support Slack channel:

So in general, non-zero codes in hooks are used to indicate that failure has occurred, and Buildkite respect this by failing the job or stopping further execution, depending on where the hook is used.

For instance: Non-zero exit codes in pre-checkout, pre-command and pre- exit hooks can cause the assotiated job fail.

This means that if any of these hooks return a non-zero exit code, the job will not proceed to the next steps and will be marked as failed. (edited)

For the post-checkout or post-command, or post-exit hooks, typically don't affect the job success or failure directly but it can be used for logging or cleanup purposes.

So thought I'd PR putting this in the docs.

I don't like documenting the negative (E.g. check mark implies job fails) so happy to hear alternative ways of succinctly documenting this.

@gilesgas
Copy link
Contributor

gilesgas commented Jun 14, 2024

Thanks very much for your contribution @thejcannon !

My apologies for taking a while to look into this.

Just some feedback before merging this in...

pages/agent/v3/hooks.md Outdated Show resolved Hide resolved
@gilesgas
Copy link
Contributor

gilesgas commented Jun 14, 2024

Just to clarify @thejcannon , does:

  • A check mark indicate a zero exit code = associated job will pass.
  • A 'X' mark indicate a non-zero exit code = associated job will fail.

?

Also, from the way this table will be presented (below), I'm not sure why some hooks only have a check mark and the others an 'X' mark? (For instance, why does the pre-bootstrap hook not have an X mark, or why does the post-checkout hook not have a check mark?)
Could you please briefly explain this distinction (and sorry I can't immediately see the significance here)?

Many thanks...

Screenshot 2024-06-14 at 11 26 01 AM

@gilesgas
Copy link
Contributor

gilesgas commented Jun 18, 2024

Sorry @thejcannon for misunderstanding the way the information was presented. I've got this now.
I feel it might be clearer just to indicate which hooks will fail and then provide a footnote description below the table.
The checkmarks (without an explanation) threw me off a little.

@gilesgas
Copy link
Contributor

Hello again @thejcannon , I'm pushed up some clarifications. Please let me know if this is what you intended...

Screenshot 2024-06-18 at 3 59 27 PM

and towards the end of the table:

Screenshot 2024-06-18 at 4 10 14 PM

@gilesgas
Copy link
Contributor

gilesgas commented Jun 18, 2024

Also, could you please confirm that it is just the post-checkout, post-command and post-artifact job hooks (which you originally indicated) that will fail upon a non-zero exit result, as this appears to be a little contradictory to what was written in the Slack extract you quoted above?

@thejcannon
Copy link
Contributor Author

Ah I like your edits! I can do the test, or you can

@thejcannon
Copy link
Contributor Author

And I think you have it inverted. The message (from buildkite support staff) indicates the post- steps don't fail job

@gilesgas
Copy link
Contributor

gilesgas commented Jun 21, 2024

And I think you have it inverted. The message (from buildkite support staff) indicates the post- steps don't fail job

Actually, I think documenting the negative has made me misunderstand what was intended, and now the latest version of what I've done (including the info callout at the end of the table) is now incorrect.
I'm following up with our support team to confirm.

If post- hooks don't fail jobs when they exit with non-zero values, then we should use ✔️ marks next to these job hooks. Using ❌ marks to mean this is somewhat counter-intuitive.

@gilesgas
Copy link
Contributor

gilesgas commented Jul 10, 2024

Hi again @thejcannon ,

Sorry for taking a while to get back to you on this.

I asked about this PR within our support team and they did a little more investigation into this topic and some testing too.

Essentially, they mentioned this (paraphrased):

If any hooks exit non-zero, the job aborts at that point.
If, for example, the command hook is the meat of the job, at the point it hits many of the post-* hooks, the command has already run to do what it needs to do, so whether or not the post-* hooks fail won't matter anymore. Although some post-* (hooks) run before the command (e.g. post-checkout), if those fail, the command wont run.

I hope this better explains the behaviour of these job lifecycle hooks, and if OK with yourself, I'll convey this information as part of this PR.

If, however, you have any other ideas on how better to convey the information you were intending, then I suggest raising these in another PR.

Thanks for your interest in contributing to our docs!

@thejcannon
Copy link
Contributor Author

That sounds great. Take the wheel! 😃

@gilesgas
Copy link
Contributor

gilesgas commented Jul 11, 2024

Thanks @thejcannon !

I've created the following PR (#2883), which supersedes this one.

Therefore, I'll close this PR and add you as a reviewer to the other one.

Edit: Hmm, it looks like I'm unable to add you as a reviewer to #2883, but if you're able to, feel free to provide your feedback on it.

@gilesgas gilesgas closed this Jul 11, 2024
@thejcannon thejcannon deleted the patch-1 branch July 11, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants