feat(modules): set variable default values #6

Balsir · 2024-12-05T10:54:53Z

Description

This PRs sets default values of internal module variables and adds automation of syncing these variables with the integration part.

The automated creation of actual integration part addon[-irsa|oidc].tf is quite tricky and will demand substantial work, so this task will be deferred to another PR.

Type of change

A bug fix (PR prefix fix)
A new feature (PR prefix feat)
A code change that neither fixes a bug nor adds a feature (PR prefix refactor)
Adding missing tests or correcting existing tests (PR prefix test)
Changes that do not affect the meaning of the code like white-spaces, formatting, missing semi-colons, etc. (PR prefix style)
Changes to our CI configuration files and scripts (PR prefix ci)
Documentation only changes (PR prefix docs)

How Has This Been Tested?

jaygridley · 2024-12-05T13:45:51Z

I got an idea for this to take it forward.

Phase 1: Manual alignment

Set the default value of all variables in modules/addon-X/variables.tf to an actual values instead of null based on the opinionated setup
Set the default value of all variables in variables-addon-X.tf to null
Set the default value from modules/addon-X/variables.tf as the last option in the "conditional" logic in addon-X.tf

Phase 2: Automation

Add a local automation that will automate phase 1 process
- Take variables from modules/addon-X/variables.tf and create the same variables in corresponding variables-addon-X.tf but setting the default to null
- Update variables-addon-X.tf and pass all variables to the module with "conditional" logic
Add GitHub action to validate output of phase 1 so we can prevent missconfigurations

Balsir · 2024-12-20T07:51:34Z

.tool-versions

@@ -4,3 +4,4 @@ tflint 0.50.3
 checkov 3.2.37
 awscli 2.15.29
 pre-commit 3.6.2
+python 3.9.16


3.9 is nearing end-of-life, can't we use newer version?
https://devguide.python.org/versions/

Yay, I should update my tool as well :-) Will do

addon-irsa.tf

Balsir · 2024-12-20T08:32:03Z

modules/addon-irsa/iam.tf

-    format("system:serviceaccount:%s:%s", var.service_account_namespace != null ? var.service_account_namespace : "", var.service_account_name != null ? var.service_account_name : "")
-  ]
+  irsa_role_create         = var.enabled && var.rbac_create && var.service_account_create && var.irsa_role_create
+  irsa_role_name_prefix    = coalesce(var.irsa_role_name_prefix, "${module.label.id}-irsa")


Why not "" default value?

"" would simplify it and I am fine with it, so be it

Balsir · 2024-12-20T14:33:23Z

modules/addon-irsa/variables.tf

-  default     = null
-  description = "IRSA role name prefix. Defaults to addon IRSA component name with `irsa` suffix."
+  default     = ""
+  description = "IRSA role name prefix. Defaults to addon IRSA component name (if provided) with `irsa` suffix."


Not sure it should construct a name if the values is ""...

Well, if the role name would end up empty, Terraform plan will fail, I think this is OK behaviour, or?

Balsir · 2024-12-20T14:34:05Z

modules/addon-irsa/variables.tf

 }

 variable "irsa_permissions_boundary" {
  type        = string
  default     = null
-  description = "ARN of the policy that is used to set the permissions boundary for the IRSA role. Defaults to `\"\"`."
+  description = "ARN of the policy that is used to set the permissions boundary for the IRSA role."


null or "" as default value?

We need null otherwise Terraform plan fails on invalid perm boundary name

Balsir · 2024-12-20T14:34:39Z

modules/addon-oidc/iam.tf

-  oidc_assume_role_enabled = var.oidc_assume_role_enabled == true && try(length(var.oidc_assume_role_arns) > 0, false)
+  oidc_provider_create     = var.enabled && var.oidc_provider_create
+  oidc_role_create         = var.enabled && var.oidc_role_create
+  oidc_role_name_prefix    = coalesce(var.oidc_role_name_prefix, "${module.label.id}-oidc")


same concern as with irsa

Balsir · 2024-12-20T14:36:07Z

modules/addon-oidc/variables.tf

 }

 variable "oidc_policy" {
  type        = string
-  default     = null
+  default     = ""


Maybe add an example of the policy format

Added more clear wording

scripts/sync-variables.py

Balsir · 2024-12-20T14:39:00Z

variables-addon.tf

 variable "argo_kubernetes_manifest_computed_fields" {
  type        = list(string)
  default     = null
-  description = "List of paths of fields to be handled as \"computed\". The user-configured value for the field will be overridden by any different value returned by the API after apply. Defaults to `[\"metadata.labels\", \"metadata.annotations\", \"metadata.finalizers\"]`."
+  description = "List of paths of fields to be handled as \"computed\". The user-configured value for the field will be overridden by any different value returned by the API after apply. Defaults to `['metadata.labels', 'metadata.annotations', 'metadata.finalizers']`."


are the single quotes ok?

I will try to improve this

Balsir · 2024-12-20T14:39:22Z

variables-addon.tf

  default     = null
-  description = "ArgoCD info manifest parameter. Defaults to `[{name=\"terraform\",value=true}]`."
+  description = "ArgoCD Application manifest info parameter. Defaults to `[{'name': 'terraform', 'value': 'true'}]`."


are the single quotes ok?

see #6 (comment)

Balsir · 2025-01-10T13:02:16Z

modules/addon-irsa/variables.tf

-  default     = null
-  description = "IRSA role name. The value is prefixed by `var.irsa_role_name_prefix`. Defaults to addon Helm chart name."
+  default     = ""
+  description = "IRSA role name. The value is prefixed by `irsa_role_name_prefix`."


comment that at least one of name or prefix have to be set

Balsir requested a review from jaygridley December 5, 2024 10:54

jaygridley force-pushed the feat/align-irsa-variables branch 2 times, most recently from b71a533 to a15e6c0 Compare December 19, 2024 13:28

jaygridley changed the title ~~feat(irsa): Align IRSA variables~~ feat(modules): set variable default values Dec 19, 2024

Balsir commented Dec 20, 2024

View reviewed changes

jaygridley self-assigned this Dec 20, 2024

jaygridley marked this pull request as ready for review December 20, 2024 15:06

jaygridley force-pushed the feat/align-irsa-variables branch from 57900cd to 5a3dd5a Compare December 20, 2024 15:08

Balsir commented Jan 10, 2025

View reviewed changes

jaygridley mentioned this pull request Jan 10, 2025

feat(modules): set variable defaults #8

Merged

7 tasks

Balsir and others added 13 commits January 10, 2025 16:43

feat(irsa): Align IRSA variables

c961008

feat: set module vars default values

bac9fc2

feat: add variable syncing

fa553ef

feat(ci): add pip caching

473fd94

feat(ci): add asdf cache

0e8d207

feat(ci): add pre-commit to pip deps cache

a3ac37f

fix: path empty string defaults

2cfcc32

docs(inputs): update

e12cf0b

feat: review improvements

97ac518

fix: integration defaults

299c75e

fix: update oidc_assume_role_policy_condition_test default to StringLike

34adb32

fix: update module source references

ce0b7c2

fix: update module source references to v0.0.12

d204cca

jaygridley force-pushed the feat/align-irsa-variables branch from 3189f60 to d204cca Compare January 10, 2025 15:50

jaygridley merged commit ccdd46e into main Jan 10, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(modules): set variable default values #6

feat(modules): set variable default values #6

Balsir commented Dec 5, 2024 •

edited by jaygridley

Loading

jaygridley commented Dec 5, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024

Balsir Dec 20, 2024

jaygridley Dec 20, 2024 •

edited

Loading

Balsir Dec 20, 2024

jaygridley Dec 20, 2024 •

edited

Loading

Balsir Jan 10, 2025

feat(modules): set variable default values #6

feat(modules): set variable default values #6

Conversation

Balsir commented Dec 5, 2024 • edited by jaygridley Loading

Description

Type of change

How Has This Been Tested?

jaygridley commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaygridley Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaygridley Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Balsir commented Dec 5, 2024 •

edited by jaygridley

Loading

jaygridley Dec 20, 2024 •

edited

Loading

jaygridley Dec 20, 2024 •

edited

Loading