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

feat: Natively support runner pre/post job hooks #4263

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

winwinashwin
Copy link
Contributor

Adds two optional string arguments to the module

  • runner_hook_job_started
  • runner_hook_job_completed

If not empty, registers the hook in user-data.sh

Ref: https://github.com/actions/runner/blob/main/docs/adrs/1751-runner-job-hooks.md

Closes: #4260
Discussion: #3854

@winwinashwin winwinashwin changed the title [Feat] Natively support runner pre/post job hooks feat: Natively support runner pre/post job hooks Nov 16, 2024
@winwinashwin winwinashwin force-pushed the ashwin/hooks branch 4 times, most recently from 0318a59 to 1182106 Compare November 16, 2024 10:59
@winwinashwin
Copy link
Contributor Author

@npalm Could you please review?

@npalm
Copy link
Member

npalm commented Nov 18, 2024

A bit annoying that the pull request checks are not showing up after the docs workflows kicks off. Maybe any clue how to fix this.

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Nice contribution! I will check the PR in detail later. But could you check if you can enrich the multi-runner example with the pre / post hook. I would suggest to use one of the amazon linux examples.

@winwinashwin
Copy link
Contributor Author

winwinashwin commented Nov 18, 2024

A bit annoying that the pull request checks are not showing up after the docs workflows kicks off. Maybe any clue how to fix this.

  1. Is it necessary to generate the docs at a PR level? Can we do a daily/weekly sync?
    If the commit is being added to the PR, then github will override the check suites with the latest commit.

  2. Or we could run the tests for all commits (even for doc)

@winwinashwin
Copy link
Contributor Author

Enrich the multi-runner example with the pre / post hook.

Thanks. Added examples in the PR.

@winwinashwin
Copy link
Contributor Author

@npalm Anything I can help with to get this merged? Thanks for your time.

@npalm
Copy link
Member

npalm commented Nov 20, 2024

@npalm Anything I can help with to get this merged? Thanks for your time.

Not much, clear my calendar. I started last week from the bottom of the PR list. Do my best to check the PR this week. Did a quicke check, see the comments. Will run asap tests based on the examples.

Comment on lines 32 to 40
runner_hook_job_started: |
echo "Running pre job hook as \$(whoami)"

# Clean github workspace from previous runs
if [[ -n "\$GITHUB_WORKSPACE" ]]; do
rm -rf "\$GITHUB_WORKSPACE"
done
runner_hook_job_completed: |
echo "Running post job hook as \$(whoami)"
Copy link
Member

Choose a reason for hiding this comment

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

can you remove the rm secion here, the runner is ephemeral so does not make sence

@@ -78,6 +78,8 @@ variable "multi_runner_config" {
cloudwatch_config = optional(string, null)
userdata_pre_install = optional(string, "")
userdata_post_install = optional(string, "")
runner_hook_job_started = optional(string, "")
Copy link
Member

Choose a reason for hiding this comment

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

can you update the documentation section for this variable please?

Copy link
Contributor Author

@winwinashwin winwinashwin Nov 21, 2024

Choose a reason for hiding this comment

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

Interesting. I was unaware that this (documentation) needs to be explicitly handled as for the other submodules docs are autogenerated. Thanks for pointing it out.

Just a thought - If the plan is to move to multi-runner by default, then maybe we should think about making this more clear. Either we should think through autogenerating docs or make it clear to contributors that documentation needs to be handled manually.

@@ -52,3 +52,12 @@ runner_config:
prefix_log_group: true
file_path: /opt/actions-runner/_diag/Runner_**.log
log_stream_name: "{instance_id}/runner"
runner_hook_job_started: |
Copy link
Member

Choose a reason for hiding this comment

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

can you remove the pre post from this example. This to have one example (linux) based without the pre post. With the two other we cover the standard (amazonn linux() as well ubuntu via custom user_data.

…bs#4260)

Pre and post job hooks were added to github actions to help
administrators run custom scripts at the beginning and end of every job.
As of today the module doesn't support these options out of the box.

Add variables to accept these optional scripts and register the hook in
user-data.

Also enrich linux-arm64 example in multi-runner with pre/post hooks

Related to: philips-labs#3854
@winwinashwin
Copy link
Contributor Author

@npalm Thank you for your review. Updated changes as suggested.

@winwinashwin
Copy link
Contributor Author

@npalm Anything pending here?

@npalm
Copy link
Member

npalm commented Nov 28, 2024

@npalm Anything pending here?

No, but my time is very limitted this week.

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.

[Feature Request] Configuration option for runner job hooks
2 participants