Fix: Remove the need for client_id and client_secret (issue #59)#60
Fix: Remove the need for client_id and client_secret (issue #59)#60nalberti wants to merge 2 commits into
Conversation
set default of null for client_id and client_secret
Removed client_id and client_secret from provider configurations defaulting to ChainedTokenCredential.
There was a problem hiding this comment.
Pull request overview
This PR aims to make the high-availability Terraform module no longer require client_id and client_secret by making those variables optional and removing them from the module-local Azure provider configurations.
Changes:
- Removed
client_idandclient_secretfromazapiandazurermprovider blocks in the high-availability module. - Made
client_idandclient_secretvariables optional by settingdefault = null.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
modules/high-availability/versions.tf |
Removes explicit Service Principal credentials from provider configuration. |
modules/high-availability/variables.tf |
Makes client_id/client_secret optional via default = null. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subscription_id = var.subscription_id | ||
| client_id = var.client_id | ||
| client_secret = var.client_secret | ||
| tenant_id = var.tenant_id |
There was a problem hiding this comment.
By removing client_id/client_secret from the azapi provider config, the module no longer supports authenticating via these module inputs (even if callers still pass them). This is a breaking behavior change vs the previous module interface and can cause unexpected auth failures for Service Principal-based deployments.
Consider either (a) wiring the (now-optional) variables into the provider config so they work when set, or (b) removing/deprecating the variables and updating module docs to clearly state credentials must come from the ambient auth chain (Azure CLI/MSI/env vars).
| tenant_id = var.tenant_id | |
| tenant_id = var.tenant_id | |
| client_id = var.client_id | |
| client_secret = var.client_secret |
| variable "client_id" { | ||
| description = "Application ID(Client ID)" | ||
| type = string | ||
| default = null | ||
| } | ||
|
|
||
| variable "client_secret" { | ||
| description = "A secret string that the application uses to prove its identity when requesting a token. Also can be referred to as application password." | ||
| type = string | ||
| sensitive = true | ||
| default = null | ||
| } |
There was a problem hiding this comment.
client_id and client_secret are now optional (default null), but they are no longer referenced anywhere in the high-availability module after the provider changes. Keeping unused inputs is misleading (and currently conflicts with the module README, which describes these as required) and makes it unclear how Service Principal auth is intended to work.
Either remove/deprecate these variables, or reintroduce their usage so setting them has an effect.
Add default values for client_id and client_secret in the high-availability module and remove them from the provider blocks.