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!: restructure module variables #723

Merged
merged 7 commits into from
Sep 7, 2023
Merged

feat!: restructure module variables #723

merged 7 commits into from
Sep 7, 2023

Conversation

kayman-mk
Copy link
Collaborator

@kayman-mk kayman-mk commented Mar 2, 2023

Description

Restructures all input variables of the agent and executor.

Closes #467 (add network_mode to docker.runner config)
Closes #513 (wait_for_services_timeout paramete)
Closes #819

Migrations required

YES. Check the script in /migrations/migrate-to-7-0-0.sh.

@kayman-mk kayman-mk requested a review from npalm as a code owner March 2, 2023 08:38
@kayman-mk kayman-mk marked this pull request as draft March 2, 2023 08:39
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Hey @kayman-mk! 👋

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.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2023

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 1 0 0.05s
✅ BASH bash-exec 1 0 0.01s
✅ BASH shellcheck 1 0 0.37s
✅ BASH shfmt 1 1 0 0.02s
✅ COPYPASTE jscpd yes no 1.77s
✅ JSON eslint-plugin-jsonc 1 0 0 1.25s
✅ JSON jsonlint 1 0 0.14s
✅ JSON prettier 1 0 0 0.37s
✅ JSON v8r 1 0 2.15s
⚠️ MARKDOWN markdownlint 4 4 1 3.05s
✅ MARKDOWN markdown-link-check 4 0 7.33s
✅ REPOSITORY checkov yes no 28.21s
✅ REPOSITORY dustilock yes no 0.33s
✅ REPOSITORY gitleaks yes no 2.28s
✅ REPOSITORY git_diff yes no 0.02s
✅ REPOSITORY grype yes no 10.58s
✅ REPOSITORY secretlint yes no 0.86s
✅ REPOSITORY syft yes no 0.35s
✅ REPOSITORY trivy-sbom yes no 0.77s
✅ REPOSITORY trufflehog yes no 5.37s
✅ SPELL cspell 29 0 3.24s
✅ TERRAFORM terraform-fmt 18 0 0 1.16s
✅ TERRAFORM terragrunt 1 0 0 0.08s
✅ YAML prettier 2 1 0 0.56s
✅ YAML v8r 2 0 3.9s
✅ YAML yamllint 2 0 0.26s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@kayman-mk kayman-mk force-pushed the refactor-variables branch from 9b4dcd4 to 3cde0f1 Compare March 16, 2023 10:47
@tmeijn
Copy link
Contributor

tmeijn commented Mar 17, 2023

@kayman-mk another thing we could possibly deprecate and remove:

/* determines if the docker machine executable adds the Name tag automatically (versions >= 0.16.2) */
# make sure to skip pre-release stuff in the semver by ignoring everything after "-"
docker_machine_version_used = split(".", split("-", var.docker_machine_version)[0])
docker_machine_version_with_name_tag = split(".", "0.16.2")
docker_machine_version_test = [
for i, j in reverse(range(length(local.docker_machine_version_used)))
: signum(local.docker_machine_version_with_name_tag[i] - local.docker_machine_version_used[i]) * pow(10, j)
]
docker_machine_adds_name_tag = signum(sum(local.docker_machine_version_test)) <= 0
}

I do not think we need to support Docker Machine < 0.16.2, which has been released ~3,5 years ago.

@tmeijn
Copy link
Contributor

tmeijn commented Mar 17, 2023

@kayman-mk #524 (comment)

Is this work you want to pick up in this iteration? Since we are doing a pretty extensive rewrite of variables already I think this is also a good one to introduce now..

@kayman-mk
Copy link
Collaborator Author

Should definately be done. See #575

@kayman-mk kayman-mk force-pushed the refactor-variables branch from 3fdd3c5 to c78907a Compare March 23, 2023 14:40
@kayman-mk kayman-mk force-pushed the refactor-variables branch 2 times, most recently from babc50a to 3662eeb Compare April 20, 2023 07:22
@kayman-mk kayman-mk force-pushed the refactor-variables branch from 26a6d19 to dc5a758 Compare April 27, 2023 06:54
@kayman-mk kayman-mk force-pushed the refactor-variables branch from dc5a758 to 7e05787 Compare May 3, 2023 06:50
@kayman-mk kayman-mk force-pushed the refactor-variables branch from 7e05787 to 824ce7a Compare May 11, 2023 08:15
@kayman-mk kayman-mk mentioned this pull request May 24, 2023
@kayman-mk kayman-mk force-pushed the refactor-variables branch from bd1cbf6 to 0f75011 Compare June 1, 2023 18:08
@kayman-mk
Copy link
Collaborator Author

Testing and bug fixing to be done in #860

@kayman-mk kayman-mk marked this pull request as ready for review June 15, 2023 19:17
@kayman-mk kayman-mk self-assigned this Jun 15, 2023
@kayman-mk kayman-mk force-pushed the refactor-variables branch 2 times, most recently from 0f75011 to f8423df Compare June 15, 2023 19:41
@kayman-mk
Copy link
Collaborator Author

@npalm I am fine with version 7 now. Could you please have a look?

@npalm
Copy link
Collaborator

npalm commented Jun 27, 2023

Will check this PR end of this week or early next week. Sorry for the delay.

@npalm
Copy link
Collaborator

npalm commented Jun 27, 2023

@kayman-mk I assume I just deploy main, next upgrade to this branch and ran the migration. Anything I should take care of?

@kayman-mk
Copy link
Collaborator Author

@npalm Yeah, that's the way it should work.

@kayman-mk kayman-mk force-pushed the refactor-variables branch 3 times, most recently from 81da9e6 to 9057ae1 Compare July 6, 2023 20:08
@kayman-mk
Copy link
Collaborator Author

Please, do not merge main into this branch. Use a rebase instead as I do not squash the commits here.

@kayman-mk kayman-mk force-pushed the refactor-variables branch from 9057ae1 to 72ffe95 Compare July 6, 2023 20:11
Copy link
Collaborator

@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.

Checked the runner-default exampl. Was not able to upgrade. Created a fix in PR #905

tmeijn and others added 7 commits September 7, 2023 17:14
## 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`.
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`

Yes. Remove the variables from your configuration. This is done
automatically by the migration script.

None.

---------

Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>

# Conflicts:
#	main.tf
…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>
Adds a new variable `runners_docker_options` which holds all values for
the `[runners.docker]` section and makes the single variables
- `runners_image`
- `runners_privileged`
- `runners_disable_cache`
- `runners_additional_volumes`
- `runners_shm_size`
- `runners_docker_runtime`
- `runners_helper_image`
- `runners_pull_policy`

obsolete.

Yes, as the minimum Terraform version is 1.3.0 to support optional block
variables with defaults.

A migration script is provided to restructure the variables. See
`/migrations/migrate-to-7-0-0.sh`. Attention Mac users: The script will
not work out of the box as the `sed` implementation is different. Use a
Docker container with Alpine or Ubuntu to run the script.

```hcl
module "gitlab_ci_runner" {
  ...
  runners_docker_options {
    # set whatever is necessary
  }
```

- [x] Use current configuration and ensure that the `config.toml`
remains unchanged
- [x] Set all new block variables and ensure that the `config.toml` is
valid (use `gitlab-runner verify)
- [x] Check that the default settings with Terraform 1.3 work as
expected
- [x] Verify all docker settings against the documentation to ensure
correct names

The runner starts in both cases and is available in Gitlab. No example
tested but used our active configuration at Hapag-Lloyd.

---------

Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>
Groups variables into objects to
- reduce the number of variables (currently 118)
- to gain a better overview of all configuration settings

Creates new groups of variables:
- `runner_manager` in case it configures the "main" process which sets
the defaults for all runners
- `runner` in case it configures the runner created by the runner
manager
- `runner_worker` in case it configures the docker/docker+machine or
leave it as it is, if it is a global scope, e.g. common tags, the
environment, ...

Yes and a script is provided to do that. It covers 98% of all migrations
(see migrations/migrate-to-7-0-0.sh)

Please mention the examples you have verified.

---------

Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>
## Description

Corrects all bugs of the pre-release version 7 of the module before
roll-out.

---------

Co-authored-by: Tyrone Meijn <tyrone_meijn@hotmail.com>
While checking the PR, I was not able to switch from main to the
refactory-varialbes branch for the example-default.

- fix a missing default
- add a null check in stated of a string check
@kayman-mk kayman-mk merged commit b8a8c1c into main Sep 7, 2023
@kayman-mk kayman-mk deleted the refactor-variables branch September 7, 2023 15:20
@kayman-mk kayman-mk restored the refactor-variables branch September 7, 2023 15:20
@kayman-mk kayman-mk deleted the refactor-variables branch October 5, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants