-
Notifications
You must be signed in to change notification settings - Fork 629
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: add time zone support for pool schedules #4063
feat: add time zone support for pool schedules #4063
Conversation
Fixes github-aws-runners#4056 Replaces the `aws_cloudwatch_event_rule` in the `pool` module (which only support UTC) with `aws_scheduler_schedule`, which supports arbitrary time zones via the `schedule_expression_timezone` attribute. The AWS EventBridge Scheduler is a drop in replacement for scheduler based AWS EventBridge Rules ([see AWS blog post](https://aws.amazon.com/blogs/compute/introducing-amazon-eventbridge-scheduler/)), including the expression syntax and pricing. The timezone can be set via the `schedule_expression_timezone` property via the main module or `multi-runners` module.
869e302
to
e086e3a
Compare
Note, I've tried to run the GitHub Action to update the auto-generated Terraform Docs (as per the contribution guide), but it's failing because it requires credentials that I don't have access to. In terms of testing, I couldn't find any existing automated tests for the pool, although I have tested it on my existing install (both with and without the new property) |
Terraform docs will updated on either release or main branch. Feel free to create an issue to address the shortcoming in the release notes
We only have automated tests for the lambda's, not for the full module. We doe typically a manual test dependending on the change with one of the examples. Would be great if we have an full auto test test. Any idea would be welcome how to run auto tests for IaC on an OS repo in a secure way. |
The process of updating the Terraform module documentation is now run on the `main` branch ([example](https://github.com/philips-labs/terraform-aws-github-runner/pull/3919)), so it no longer needs to be run on forked repositories (it also requires credentials for the GitHub App). As per @npalm's [comment](https://github.com/philips-labs/terraform-aws-github-runner/pull/4063#issuecomment-2286734539), I'm removing this reference, to prevent any confusion for new contributors.
👍 I've created a PR to remove the reference from the contribution guide, as it's no longer relevant when creating a PR. |
Unfortunately, I'm not aware of any magic solution. For internal apps, I've used Terratest to spin up ephemeral environments that are realistic, then run integration tests against them. I think that'd be tricky for this project though, especially given the fact it's a public project. One possibility could be to use Terraform's native test features to plan the example modules (using mock providers, so no AWS access is required), then running assertions against it. For example, asserting that all of the |
Thanks for the feedback! That was also my take and the reason why there no intergration tests (yet). Internally for the runners we deploy to a staging and ran validation workflows as verification. All automated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, nice contribution. Tiny suggestion.
Tested with ephemeral example, works as expected. |
The process of updating the Terraform module documentation is now run on the `main` branch ([example](https://github.com/philips-labs/terraform-aws-github-runner/pull/3919)), so it no longer needs to be run on forked repositories (it also requires credentials for the GitHub App). As per @npalm's [comment](https://github.com/philips-labs/terraform-aws-github-runner/pull/4063#issuecomment-2286734539), I'm removing this reference, to prevent any confusion for new contributors.
Co-authored-by: Niek Palm <[email protected]>
Thanks for the review, I've accepted your suggestion |
Thanks for adding the feature! Feel free to update the other time triggers as well! |
🤖 I have created a release *beep* *boop* --- ## [5.15.0](philips-labs/terraform-aws-github-runner@v5.14.1...v5.15.0) (2024-08-16) ### Features * add time zone support for pool schedules ([#4063](https://github.com/philips-labs/terraform-aws-github-runner/issues/4063)) ([b8f9eb4](philips-labs/terraform-aws-github-runner@b8f9eb4)) @janslow * scale up for long waiting jobs (job retry) ([#4064](https://github.com/philips-labs/terraform-aws-github-runner/issues/4064)) ([6120571](philips-labs/terraform-aws-github-runner@6120571)) ### Bug Fixes * **lambda:** bump axios from 1.7.2 to 1.7.4 in /lambdas ([#4071](https://github.com/philips-labs/terraform-aws-github-runner/issues/4071)) ([2f32195](philips-labs/terraform-aws-github-runner@2f32195)) * **lambda:** bump the aws group in /lambdas with 5 updates ([#4057](https://github.com/philips-labs/terraform-aws-github-runner/issues/4057)) ([5ecdbad](philips-labs/terraform-aws-github-runner@5ecdbad)) * **lambda:** bump the aws-powertools group in /lambdas with 3 updates ([#4058](https://github.com/philips-labs/terraform-aws-github-runner/issues/4058)) ([f9533f3](philips-labs/terraform-aws-github-runner@f9533f3)) * **lambda:** Prevent scale-up lambda from starting runner for user repo if org level runners is enabled ([#3909](https://github.com/philips-labs/terraform-aws-github-runner/issues/3909)) ([98b1560](philips-labs/terraform-aws-github-runner@98b1560)) @PerGon --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: forest-releaser[bot] <80285352+forest-releaser[bot]@users.noreply.github.com>
## Problme In #4063 PR the EventBridge Schedule is introduces. For schedule groups a name prefix is used. But since the name for the name prefix is already unieque the name can be used instead of prefix to avoid too long prefix names (30 chars plus).
Fixes #4056
The timezone can be set via the
schedule_expression_timezone
property via the main module ormulti-runners
module.Replaces the
aws_cloudwatch_event_rule
in thepool
module (which only support UTC) withaws_scheduler_schedule
, which supports arbitrary time zones via theschedule_expression_timezone
attribute.In this situation AWS EventBridge Scheduler is a drop in replacement for schedule-based AWS EventBridge Rules (see AWS blog post), including the expression syntax and pricing. The main (internal) difference is that AWS Scheduler requires the use of an IAM Role, instead of Lambda permissions.