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

Minor: Standardise variable names #575

Closed
baolsen opened this issue Nov 17, 2022 · 11 comments
Closed

Minor: Standardise variable names #575

baolsen opened this issue Nov 17, 2022 · 11 comments
Labels
enhancement 🆕 New feature or request

Comments

@baolsen
Copy link
Contributor

baolsen commented Nov 17, 2022

Some variable names could be standarised.

First example, all of the below relate to cloudwatch log groups:
log_group_name
enable_cloudwatch_logging
create_cloudwatch_log_group
cloudwatch_logging_retention_in_days

Probably best to leave this issue for a little while to collect and discuss other cases of standardisation and then to a single PR to resolve.

It may be helpful to also compile a "migration list" so that users can easily upgrade between different named variables if there are many.

Note, changing these would be breaking changes.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

This issue 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.

@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Mar 2, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label Mar 2, 2023
@kayman-mk kayman-mk added this to the Rework variables milestone Mar 2, 2023
@kayman-mk
Copy link
Collaborator

First proposal:

module "gitlab_runner" {
  agent {
    instance_type = "t3.micro"
    ebs_optimized = true
    enable_monitoring = true

    # et al.
  }

  executor {
    egress_rules = [ ...]
    ami_owners = [ ... ]

    cache_bucket {
    }

    # et al.
  }

  vpc_id = "abc"
  permissions_boundary = "xyz"
  enable_kms = true

  # et al.
}

@kayman-mk
Copy link
Collaborator

I prepared a long lived feature branch for this. See #723

@kayman-mk
Copy link
Collaborator

@npalm What do you think about the above mentioned restructuring? If you are fine with the agent and executor blocks I would work on the renaming of the variables.

@tmeijn
Copy link
Contributor

tmeijn commented Mar 23, 2023

Hey @kayman-mk I had a proposal/thought, let me know what you think?

I was actually thinking, why don't remap all variables related to the config.toml to Terraform object variables?

We've already been pretty successful imho in #511 and #711 using this pattern, so I was thinking why don't we use the same pattern for all sections of the config.toml?

module "gitlab_runner" {
# other options....

<good_prefix>_config_global_section = {
  # all options we want configurable in the global section of the `config.toml`. Corresponds to https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-global-section
}

<good_prefix>_config_runners_section = {
  # Corresponds to https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runners-section
}

<good_prefix>_config_runners_docker_section = {
  # Corresponds to https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersdocker-section
}

<good_prefix>_config_runners_machine_section = {
  # Corresponds to https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersmachine-section
}

<good_prefix>_config_runners_machine_section_machine_options = {
  # Corresponds to https://gitlab.com/gitlab-org/ci-cd/docker-machine/-/blob/main/drivers/amazonec2/amazonec2.go#L128
}

https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersdocker-section

}

That would:

  • reduce a lot of the input variables while making it easier (imo) to add additional properties when to become available
  • Make a pretty good mapping between the inputs of this module and the actual GitLab Runner Documentation. We could then more easily just refer the user to the official GitLab documentation.

@kayman-mk
Copy link
Collaborator

I got rid of the maps and used the map name as additional prefix. This way we have single variables, which can be documented.

Good point: Use the GitLab documentation as documentation for these variables.

@tmeijn
Copy link
Contributor

tmeijn commented Mar 23, 2023

I think documenting as "Corresponds to the

in the GitLab Runner config. See for options and explanation" should be good enough documentation if you do in the proposed way? 🤔

variable "<good_prefix>_config_global_section" {
  description = "Corresponds to the Global section in the GitLab Runner config. See https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-global-section for available options and explanation"
}

We could then also add a multi-line doc explaining the case conversion scheme and which options are not available due to whatever reason.

variable "<good_prefix>_config_runners_machine_section" {
  description = <<EOF
     Corresponds to the [[runners.machine]] section in the GitLab Runner config. See https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runnersmachine-section
     
     Note that not all options are available, see the object definition for available options
  EOF
}

Just for your consideration, not meant to be pushy 😄

@github-actions
Copy link
Contributor

This issue 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.

@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label May 23, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label May 24, 2023
@github-actions
Copy link
Contributor

This issue 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.

@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Jul 24, 2023
@npalm npalm removed the stale Issue/PR is stale and closed automatically label Jul 24, 2023
@github-actions
Copy link
Contributor

This issue 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.

@github-actions github-actions bot added the stale Issue/PR is stale and closed automatically label Sep 23, 2023
@kayman-mk kayman-mk removed the stale Issue/PR is stale and closed automatically label Sep 28, 2023
@kayman-mk
Copy link
Collaborator

Fixed with the 7.0.0 release of the module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants