terraform-aws-gitlab-runner
terraform-aws-gitlab-runner copied to clipboard

feat: allow to set all docker options for the Executor

Open kayman-mk opened this issue 2 years ago • 15 comments

Description

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. The new runners_docker_options can be activated by setting the runners_enable_docker_options to true.

Closes #467, #491, #513

Migrations required

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

Recommended for a cleaner configuration. Replace the above mentioned variables by the new map

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

Verification

[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)
[ ] Check that the default settings with Terraform 1.3 work as expected

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

Jun 19 '22 18:06 kayman-mk

Make sure to write the toml file with correct data types, e.g. boolean and numbers without ""

Jun 22 '22 21:06 kayman-mk

@npalm Not a good idea? There are so many variables and many more to come as the issues here indicate (#467, #491, #513). Thought it would be better to place them in a map for a cleaner module interface.

Jun 22 '22 21:06 kayman-mk

A block would be an alternative solution.

runners_docker_options {
  shm_size =0
}

Jun 23 '22 04:06 kayman-mk

Could we get rid of the runners_enable_docker_options variable and maintain backwards compatibility by making the default just use the old variables?

We'd have to note on the old variables that if runners_docker_options is set, then they're ignored, but that'd simplify a lot of the code here.

Jun 24 '22 05:06 JulianCBC

Sure. But setting a default for the new block is problematic. I have to know whether the new block is used or the old variables.

We could set a default null and then decide: if var.runners_docker_options == null then use old variables else use new block variables.

As soon as the old variables are removed we could set the default for the block.

Jun 24 '22 20:06 kayman-mk

Other than enabling the experiment which I can't see actually being used right now, this looks good to me!

I can't help but wonder if you could use the template for both the original and new variable sets, and do all the formatting of the volumes string in the template, but those are pretty minor quibbles and the code I'm complaining about is deprecated.

Jun 26 '22 22:06 JulianCBC

Yeah, the experiment is not my favorite too. But it made it into Terraform 1.3.0 (released soon). Default values will also be possible.

Jun 27 '22 17:06 kayman-mk

Good point though. Reworked that section. Looks much better now.

Jun 27 '22 18:06 kayman-mk

Looks better but doesn't work. I always access null variables. Reverted.

Jun 27 '22 19:06 kayman-mk

Would be great when we can move to Terrfaform 1.3. The optiional in object will made the code a lot cleaner on my places. Just checked, there are two alphaversions out. So it is still in an early state but at least we see the progress towards 1.3

Jul 16 '22 08:07 npalm

Still waiting for the Terraform 1.3 release. No idea when this will happen.

Aug 09 '22 18:08 kayman-mk

Terraform 1.3 was released some days ago.

Oct 12 '22 18:10 kayman-mk

Terraform 1.3 was released some days ago.

Yes and now we can use finally optionals with defaults

Oct 13 '22 19:10 npalm

@kayman-mk please can you rebase the PR with develop?

Oct 13 '22 21:10 npalm

Did a quick check with default example, but got the error ERROR: Preparation failed: unsupported pull_policy config: ""

Oct 13 '22 21:10 npalm

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 15 days.

Feb 10 '23 03:02 github-actions[bot]

@npalm Any idea why the Terraform workflows do not run in this branch? I merged the current main already.

EDIT: Strange. Running now.

Mar 02 '23 07:03 kayman-mk

Tested with my converted configuration. Jobs are still processed.

Mar 02 '23 10:03 kayman-mk

As soon as we are satisfied, we should release it with #723, I think. This saves one major release and it fits perfectly into that PR.

Mar 11 '23 19:03 kayman-mk

I changed the PR description and mention the problem on Macs. Possible solution is to run the script in a Docker container.

Let me do a test on my machine again to make sure that everything is running fine. I already doublechecked the variable names with the handbook. They look ok.

Mar 11 '23 19:03 kayman-mk

I verified the config.toml by adding all variables manually according to the template file. Names and types were correct. I am good to go.

Mar 11 '23 20:03 kayman-mk

Hi, I just became aware of this Pull Request and I got a couple of remarks/suggestions.

While this makes the interface for the user a lot simpler and gives more capabilities/settings to the user, I'm a bit worried about the complexity this introduces into the module itself.

Instead of parsing each variable from the object separately we can pass the key+value pair as is to the runners_docker_options.tfpl since we control both the key and value type with the object definition. Within the template itself we have to accommodate for only three variable types:

Arrays/lists
numbers
strings

I tried to do this in my own PR, but unfortunately I didn't get any feedback from you all yet. But it seems you are trying to do the same. See my draft PR #711 and specifically the logic in https://github.com/cattle-ops/terraform-aws-gitlab-runner/pull/711/files#diff-fd9e1a870839d390694bebe28698eddedcb6a97b768004316ce5c27b78811181

Something like this should probably work (but not tested):

%{~ for key, value in config ~}
      %{ if contains([LIST_OF_VALUES_TO_CONVERT_TO_TOML_ARRAY], key) }${key} = [${replace(format("\"%s\"", join("\",\"", value)), "/\"{2,}/", "\"")}]%{ else }
      ${key} = ${ can(tonumber(value)) ? value : format("\"%s\"", value)}
      %{~ endif ~}
%{~ endfor ~}

This should drastically reduce the parsing that's happening now in locals.tf and should also remove most if not all if statements for each key in the template file.

Mar 12 '23 18:03 tmeijn

Sounds good to me. I saw this problem during development but didn't find a solution. I will try yours. It should make the configuration a lot easier.

Mar 12 '23 21:03 kayman-mk

@kayman-mk actual we can and should even keep the logic out of the template. We can use for ... in ... to convert the values to TOML compatible values. I've found a generic "function" we could use to basically map HCL values to TOML values. See below for a quick and dirty experiment.

"function":

runners_machine_autoscaling = templatefile("${path.module}/template/runners_machine_autoscaling.tpl", {
    runners_machine_autoscaling = [for entry in var.runners_machine_autoscaling :
      { for option, value in entry : option => (
        can(distinct(value)) ?                                                     # distinct can only be used on lists. In combination with 'can' we determine whether the current value is a list or other.
        "[${replace(format("\"%s\"", join("\",\"", value)), "/\"{2,}/", "\"")}]" : # Wrap and convert to TOML array
        can(tonumber(value)) ? value : format("\"%s\"", value)                     # Leftover value is either a number or string. If string, we wrap with qoutes.
        ) if value != null

    }]
    }
  )

template:

%{~ for config in runners_machine_autoscaling ~}
    [[runners.machine.autoscaling]]
    %{~ for key, value in config ~}
      ${key} = ${value}
      %{~ endfor ~}
%{~ endfor ~}

input:

  runners_machine_autoscaling = [
    {
      an_array_with_str = ["one", "two", "three"]
      a_number = 1
      a_string = "hello, World!"
    }
  ]

output:

If this works and is robust I see a chance to greatly simplify all the logic we do now in and outside the config.toml template.

Mar 13 '23 08:03 tmeijn

@npalm @tmeijn Looks much better now, right? Thanks for the hint.

Mar 13 '23 19:03 kayman-mk

@kayman-mk did something go wrong here? This PR shows it's now empty and merging refactor-variables in #711 shows changes that I think were in this PR.

Mar 16 '23 13:03 tmeijn

Oh no, what happened here?! I should have a local copy.

Mar 16 '23 19:03 kayman-mk

Ok, back on track.

Mar 16 '23 19:03 kayman-mk

Can this be finalized and merged soon? We need to change wait_for_services_timeout param in runners.docker section and it appears there is no way to do so until this PR is merged. Thank you.

Mar 19 '23 18:03 demisx

FYI: services_security_opt made it into GitLab Runner 15.10 today... https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/3760. It's not documented yet though...

Mar 20 '23 14:03 tmeijn