feat!: add idle_count_min `and` idle_scale_factor` to Docker Machine autoscaling options #711

tmeijn · 2023-02-25T22:07:16Z

Description

Switches from hardcoded options to free-from scaling configuration. This reduces the module complexity by allowing to get rid of a number of variables while giving more control to the user to define their options without us having to build support into it for.

Adds idle_scale_factor and idle_count_min Docker Machine options. See documentation.

Migrations required

YES - users will have to change the input name from runners_machine_autoscaling to runners_machine_autoscaling_options. No other changes should be needed, we just support more options. A migration script is available.

Verification

No input given:

(end of rendered config.toml)

Input:

runners_machine_autoscaling_options = [
    {
      periods           = ["* * 9-17 * * mon-fri *", "* * 9-17 * * mon-fri *"]
      idle_count        = 50
      idle_count_min    = 10
      idle_time         = 3600
      timezone          = "UTC"
      idle_scale_factor = 1.5
    },
    {
      periods    = ["* * 9-17 * * mon-fri *", "* * 9-17 * * mon-fri *"]
      idle_count = 50
      idle_time  = 3600
      timezone   = "Europe/Amsterdam"
    }
  ]

Rendered config.toml:

Apply results:

Closes #556

github-actions · 2023-02-25T22:07:28Z

Hey @tmeijn! 👋

Thank you for your contribution to the project. Please refer to the contribution rules for a quick overview of the process.

Make sure that this PR clearly explains:

the problem being solved
the best way a reviewer and you can test your changes

With submitting this PR you confirm that you have the rights of the code added and agree that it will published under the MIT license.

This message was generated automatically. You are welcome to improve it.

tmeijn · 2023-02-25T22:14:53Z

Hey @npalm, @kayman-mk, this PR is totally not ready, but wanted to get yall's opinion on whether you think this is even an acceptable or desired direction to go in. Played around with it a bit and

Produces

and

npalm · 2023-02-26T09:45:13Z

Can you set the PR in "draft" as long it is not ready?

…into feat/switch-to-free-form-scaling-config

## Description Removes the earlier deprecated `runners_pull_policy` variable. Since were making a Major release I thought this one was nice to catch. ## Migrations required YES. Replace the `runners_pull_policy` by `runners_pull_policies`.

kayman-mk · 2023-03-11T18:47:45Z

@tmeijn Could you please draft the changes to the input variables? There is another PR in the pipeline to restructure everything. Would be good to combine both.

See #723

…aws-gitlab-runner into feat/switch-to-free-form-scaling-config

kayman-mk · 2023-03-13T19:29:26Z

Makes sense to me to support the new settings. But I suggest to use idle_count instead of IdleCount, ... to have a consistent naming through out the module.

tmeijn · 2023-03-14T09:14:10Z

Makes sense to me to support the new settings. But I suggest to use idle_count instead of IdleCount, ... to have a consistent naming through out the module.

Yeah this is because the naming convention in the config.toml is very inconsistent... but I was doubting between passing the keys as is or us providing the conversion.

## Description Removes the earlier deprecated `runners_pull_policy` variable. Since were making a Major release I thought this one was nice to catch. ## Migrations required YES. Replace the `runners_pull_policy` by `runners_pull_policies`.

kayman-mk · 2023-03-16T10:00:05Z

I am good to go with the naming and add an action to the unit test as the names from the variables are copied 1:1 to the configuration file.

tmeijn · 2023-03-16T10:18:13Z

@kayman-mk I was actually thinking to write a conversion from snake_case to CamelCase, sorry didn't make that intent clear in my previous comment. Do you think that would be better?

kayman-mk · 2023-03-16T10:39:05Z

I really appreciate such a converter, but I guess it complicates the code as there is no function available in Terraform. The related issue hashicorp/terraform#25230 is still open for a while.

I think it is better to leave the code as it is now and add some unit tests. Gosh, I have to get #632 ready now.

tmeijn · 2023-03-16T11:45:01Z

Hey @kayman-mk I just tried it, see c3cf08c.

I personally don't find it too complex looking and if it helps you with the testing I'm happy to implement it like this, but I'll let you decide what you would prefer based on this example.

## Description This PR removes all variables which are marked as deprecated. - `arn_format` - `subnet_id_runners` - `subnet_ids_gitlab_runner` - `asg_terminate_lifecycle_hook_create` - `asg_terminate_lifecycle_hook_heartbeat_timeout` - `asg_terminate_lifecycle_lambda_memory_size` - `asg_terminate_lifecycle_lambda_runtime` - `asg_terminate_lifecycle_lambda_timeout` ## Migrations required Yes. Remove the variables from your configuration. This is done automatically by the migration script. ## Verification None. --------- Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>

…aws-gitlab-runner into feat/switch-to-free-form-scaling-config

tmeijn · 2023-03-17T11:35:27Z

@kayman-mk I think this PR is ready for review. I do not know what's up with the Lint PR title action, are you able to help?

kayman-mk · 2023-03-19T08:59:16Z

@tmeijn KICS is reporting problems. Please fix them. Lint PR title: Works for me, I guess because I am a member of cattle-ops. Have to dig in a little bit further. Guess a PAT will help for forks outside the organization.

Could you please enhance this script and add a conversion for your renaming?

tmeijn · 2023-03-23T12:32:27Z

@kayman-mk Regarding the KICS errors, I see you already fixed them in #755, so I think once that is merged the errors reported here will automatically resolve.

Regarding the PR lint: it did seem to work in my first contribution I made at least: https://github.com/cattle-ops/terraform-aws-gitlab-runner/pull/705/checks#step:2:1

## Description Removes the earlier deprecated `runners_pull_policy` variable. Since were making a Major release I thought this one was nice to catch. ## Migrations required YES. Replace the `runners_pull_policy` by `runners_pull_policies`.

## Description This PR removes all variables which are marked as deprecated. - `arn_format` - `subnet_id_runners` - `subnet_ids_gitlab_runner` - `asg_terminate_lifecycle_hook_create` - `asg_terminate_lifecycle_hook_heartbeat_timeout` - `asg_terminate_lifecycle_lambda_memory_size` - `asg_terminate_lifecycle_lambda_runtime` - `asg_terminate_lifecycle_lambda_timeout` ## Migrations required Yes. Remove the variables from your configuration. This is done automatically by the migration script. ## Verification None. --------- Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>

…ng-config

npalm · 2023-04-16T14:12:09Z

The PR linter is failing since the fork have no access to the write token used in the workflow.

…ng-config

kayman-mk

Thanks for your work!

…autoscaling options (#711) ## Description Switches from hardcoded options to free-from scaling configuration. This reduces the module complexity by allowing to get rid of a number of variables while giving more control to the user to define their options without us having to build support into it for. Adds `idle_scale_factor` and `idle_count_min` Docker Machine options. See [documentation](https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersmachine-section"). ## Migrations required YES - users will have to change the input name from `runners_machine_autoscaling` to `runners_machine_autoscaling_options`. No other changes should be needed, we just support _more_ options. A migration script is available. ## Verification No input given: (end of rendered `config.toml`) ![image](https://user-images.githubusercontent.com/17970041/225890782-02fe4adc-4c6a-4237-9752-a64349464113.png) Input: ```hcl runners_machine_autoscaling_options = [ { periods = ["* * 9-17 * * mon-fri *", "* * 9-17 * * mon-fri *"] idle_count = 50 idle_count_min = 10 idle_time = 3600 timezone = "UTC" idle_scale_factor = 1.5 }, { periods = ["* * 9-17 * * mon-fri *", "* * 9-17 * * mon-fri *"] idle_count = 50 idle_time = 3600 timezone = "Europe/Amsterdam" } ] ``` Rendered `config.toml`: ![image](https://user-images.githubusercontent.com/17970041/225891085-add03ee8-3943-4c56-96a4-d1a8c252deb0.png) Apply results: ![image](https://user-images.githubusercontent.com/17970041/225893020-a9850486-4aa6-4eb0-b996-558ec7bccfea.png) Closes #556 --------- Co-authored-by: Matthias Kay <github-public@matthiaskay.de>

🤖 I have created a release *beep* *boop* --- ## [7.0.0](6.5.2...7.0.0) (2023-09-09) ### ⚠ BREAKING CHANGES * group variables for better overview ([#810](#810)) * allow to set all docker options for the Executor ([#511](#511)) * add idle_count_min` and `idle_scale_factor` to Docker Machine autoscaling options ([#711](#711)) * remove deprecated variables ([#738](#738)) * remove deprecated pull policy variable ([#710](#710)) ### Features * add idle_count_min` and `idle_scale_factor` to Docker Machine autoscaling options ([#711](#711)) ([1538d48](1538d48)) * allow to set all docker options for the Executor ([#511](#511)) ([461561e](461561e)) ### Bug Fixes * add missing defaults ([#905](#905)) ([eb44182](eb44182)) * correct the bugs of major version 7 (pre-release) ([#860](#860)) ([f236b58](f236b58)) * remove deprecated pull policy variable ([#710](#710)) ([8736ec7](8736ec7)) ### Miscellaneous Chores * remove deprecated variables ([#738](#738)) ([676ed6a](676ed6a)) ### Code Refactoring * group variables for better overview ([#810](#810)) ([c8a3b89](c8a3b89)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: Niek Palm <dev.npalm@gmail.com> Co-authored-by: cattle-ops-releaser-2[bot] <134548870+cattle-ops-releaser-2[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

tmeijn added 2 commits February 25, 2023 22:31

feat\!: switch to free-from scaling configurations

848a00b

add some validation

669e944

tmeijn and others added 2 commits February 26, 2023 17:04

Merge branch 'main' of github.com:tmeijn/terraform-aws-gitlab-runner …

2d87d68

…into feat/switch-to-free-form-scaling-config

tmeijn mentioned this pull request Mar 12, 2023

feat!: allow to set all docker options for the Executor #511

Merged

4 tasks

tmeijn changed the base branch from main to refactor-variables March 13, 2023 08:32

tmeijn added 7 commits March 13, 2023 09:42

Merge branch 'refactor-variables' of github.com:cattle-ops/terraform-…

159e29e

…aws-gitlab-runner into feat/switch-to-free-form-scaling-config

type variable and use generic for-in conversion

6bfba05

remove testing variable

0122aa8

set correct tf version and variable

2890ca4

fmt and input fix

cf4f794

rename output

607c499

fmt...

89e6d84

kayman-mk force-pushed the refactor-variables branch from 1bbbea3 to a2f0c60 Compare March 13, 2023 19:46

kayman-mk force-pushed the refactor-variables branch from a2f0c60 to 3cde0f1 Compare March 16, 2023 07:57

kayman-mk mentioned this pull request Mar 16, 2023

Add unit tests #721

Open

convert snake_case to PascalCase and easy convert value

c3cf08c

kayman-mk and others added 2 commits March 16, 2023 22:59

Merge branch 'refactor-variables' of github.com:cattle-ops/terraform-…

4ca78d5

…aws-gitlab-runner into feat/switch-to-free-form-scaling-config

tmeijn requested review from npalm and kayman-mk as code owners March 17, 2023 11:34

kayman-mk added this to the Rework variables (major release) milestone Mar 19, 2023

tmeijn mentioned this pull request Mar 23, 2023

Minor: Standardise variable names #575

Closed

tmeijn added 2 commits March 23, 2023 11:40

add cspell words and migration sed

1d1549f

addtional cspell out of nowhere?

ec4c534

tmeijn and others added 2 commits March 23, 2023 15:40

kayman-mk force-pushed the refactor-variables branch from 3fdd3c5 to c78907a Compare March 23, 2023 14:40

Merge branch 'refactor-variables' into feat/switch-to-free-form-scali…

8b3e7ad

…ng-config

kayman-mk force-pushed the refactor-variables branch from c78907a to 93657e6 Compare April 20, 2023 06:17

Merge branch 'refactor-variables' into feat/switch-to-free-form-scali…

90dcbcc

…ng-config

kayman-mk changed the title ~~feat!: refactor Docker Machine autoscaling options~~ feat!: add idle_count_min and idle_scale_factor` to Docker Machine autoscaling options Apr 20, 2023

kayman-mk approved these changes Apr 20, 2023

View reviewed changes

kayman-mk merged commit babc50a into cattle-ops:refactor-variables Apr 20, 2023

cattle-ops-releaser-2 bot mentioned this pull request Sep 7, 2023

chore(main): release 7.0.0 #954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add idle_count_min `and` idle_scale_factor` to Docker Machine autoscaling options #711

feat!: add idle_count_min `and` idle_scale_factor` to Docker Machine autoscaling options #711

tmeijn commented Feb 25, 2023 •

edited by kayman-mk

Loading

github-actions bot commented Feb 25, 2023

tmeijn commented Feb 25, 2023

npalm commented Feb 26, 2023

kayman-mk commented Mar 11, 2023

kayman-mk commented Mar 13, 2023

tmeijn commented Mar 14, 2023

kayman-mk commented Mar 16, 2023

tmeijn commented Mar 16, 2023 •

edited

Loading

kayman-mk commented Mar 16, 2023

tmeijn commented Mar 16, 2023 •

edited

Loading

tmeijn commented Mar 17, 2023 •

edited

Loading

kayman-mk commented Mar 19, 2023 •

edited

Loading

tmeijn commented Mar 23, 2023

npalm commented Apr 16, 2023

kayman-mk left a comment

feat!: add idle_count_min and idle_scale_factor` to Docker Machine autoscaling options #711

feat!: add idle_count_min and idle_scale_factor` to Docker Machine autoscaling options #711

Conversation

tmeijn commented Feb 25, 2023 • edited by kayman-mk Loading

Description

Migrations required

Verification

github-actions bot commented Feb 25, 2023

tmeijn commented Feb 25, 2023

npalm commented Feb 26, 2023

kayman-mk commented Mar 11, 2023

kayman-mk commented Mar 13, 2023

tmeijn commented Mar 14, 2023

kayman-mk commented Mar 16, 2023

tmeijn commented Mar 16, 2023 • edited Loading

kayman-mk commented Mar 16, 2023

tmeijn commented Mar 16, 2023 • edited Loading

tmeijn commented Mar 17, 2023 • edited Loading

kayman-mk commented Mar 19, 2023 • edited Loading

tmeijn commented Mar 23, 2023

npalm commented Apr 16, 2023

kayman-mk left a comment

Choose a reason for hiding this comment

feat!: add idle_count_min `and` idle_scale_factor` to Docker Machine autoscaling options #711

feat!: add idle_count_min `and` idle_scale_factor` to Docker Machine autoscaling options #711

tmeijn commented Feb 25, 2023 •

edited by kayman-mk

Loading

tmeijn commented Mar 16, 2023 •

edited

Loading

tmeijn commented Mar 16, 2023 •

edited

Loading

tmeijn commented Mar 17, 2023 •

edited

Loading

kayman-mk commented Mar 19, 2023 •

edited

Loading