Skip to content

feat: accept snake_case environment variable names - #298

Open
Abhinesh Jha (Abhineshhh) wants to merge 3 commits into
Azure:mainfrom
Abhineshhh:feat/248-snake-case-env-vars
Open

feat: accept snake_case environment variable names#298
Abhinesh Jha (Abhineshhh) wants to merge 3 commits into
Azure:mainfrom
Abhineshhh:feat/248-snake-case-env-vars

Conversation

@Abhineshhh

Copy link
Copy Markdown

Summary

Implements support for snake_case environment variable names while fully preserving the existing concatenated names for backward compatibility.

Closes / fixes:#248

Problem

Environment variables used a hard-to-read concatenated form (e.g. MPF_SUBSCRIPTIONID). Users requested conventional snake_case (e.g. MPF_SUBSCRIPTION_ID) without breaking existing scripts and CI.

Solution

  • Derive both env names from each Cobra flag:
    • Legacy:MPF_ + upper(flagName) → MPF_SUBSCRIPTIONID (unchanged behavior)
    • Snake_case:MPF_ + upper(camelToSnake(flagName)) → MPF_SUBSCRIPTION_ID
  • Bind both via Viper BindEnv for every flag (global, ARM, Bicep, Terraform)
  • Precedence: CLI flags > legacy env > snake_case env
  • Document preferred + legacy forms in the flag reference
  • E2E tests accept either form via a small mpfEnv() helper (legacy first)

Examples

FlagPreferred envLegacy env (still works)
subscriptionIDMPF_SUBSCRIPTION_IDMPF_SUBSCRIPTIONID
spClientIDMPF_SP_CLIENT_IDMPF_SPCLIENTID
templateFilePathMPF_TEMPLATE_FILE_PATHMPF_TEMPLATEFILEPATH
tfPathMPF_TF_PATHMPF_TFPATH

Changes

AreaFiles
CLI bindingcmd/envbinding.go, cmd/rootCmd.go
Unit testscmd/envbinding_test.go
Docsdocs/commandline-flags-and-env-variables.md, README.md, AGENTS.md
E2E alignmente2eTests/env_helpers.go + e2e test files

Backward compatibility

  • Existing MPF_SUBSCRIPTIONID, MPF_TFPATH, etc. continue to work
  • If both forms are set, legacy wins so current deployments are not surprised
  • No flag renames, no dependency changes, no changes to deploy/permission logic under pkg/

Test plan

  • go test ./cmd/ -count=1
  • go test ./pkg/... -count=1
  • go test ./e2eTests/ -count=1 (compile / unit path; live Azure e2e not run in this environment)
  • go vet ./cmd/ ./e2eTests/
  • gofmt clean on touched Go files
  • Unit coverage for: snake_case only, legacy only, both set (legacy wins), CLI overrides env, terraform flags, initializeConfig with root+arm flags

References

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for snake_case environment variables across the azmpf CLI (via Cobra/Viper), while preserving and prioritizing the existing legacy concatenated env var names for backward compatibility (per issue #248).

Changes:

  • Implement dual env var binding per flag (legacy + snake_case) with legacy precedence via centralized binding logic in cmd/envbinding.go.
  • Add unit coverage for camel→snake conversion, env name derivation, and binding precedence/behavior.
  • Align docs and E2E tests to accept either env var form (legacy-first lookup helper).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
FileDescription
README.mdUpdates examples to prefer snake_case env vars while noting legacy compatibility.
docs/commandline-flags-and-env-variables.mdDocuments snake_case vs legacy naming rules and lists both env var forms per flag.
cmd/rootCmd.goSwitches config initialization to use the new dual-env binding helper.
cmd/envbinding.goIntroduces camel→snake conversion, env name derivation, and dual BindEnv behavior.
cmd/envbinding_test.goAdds unit tests for conversion, env name generation, and precedence (CLI > legacy env > snake_case).
e2eTests/env_helpers.goAdds mpfEnv() helper to read legacy-first, then snake_case env vars in tests.
e2eTests/e2eTerraformWithImportAndTargeting_test.goUpdates terraform path env lookup to accept both env var forms.
e2eTests/e2eTerraformInvalid_test.goUpdates terraform path env lookup to accept both env var forms.
e2eTests/e2eTerraformAuthPermissionMismatch_test.goUpdates terraform path env lookup to accept both env var forms.
e2eTests/e2eTerraformAuthorizationRequestDenied_test.goUpdates terraform path env lookup to accept both env var forms.
e2eTests/e2eTerraform_test.goUpdates terraform path env lookup to accept both env var forms.
e2eTests/e2eBicepInvalid_test.goUpdates bicep exec path env lookup to accept both env var forms.
e2eTests/e2eBicep_test.goUpdates bicep exec path env lookup to accept both env var forms.
e2eTests/e2eArm_test.goUpdates ARM E2E env var reads to accept both legacy and snake_case forms.
AGENTS.mdUpdates agent instructions/examples to use snake_case env vars (preferred).

@maniSbindra

Copy link
Copy Markdown
Contributor

Abhinesh Jha (@Abhineshhh) Thanks for the PR. It looks good. Can you look that linting PR check which is failing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abhinesh Jha (@Abhineshhh) Thanks for the PR. It looks good. Can you look that linting PR check which is failing

Support preferred snake_case env vars (e.g. MPF_SUBSCRIPTION_ID) while
keeping legacy concatenated names (e.g. MPF_SUBSCRIPTIONID) for backward
compatibility. Bind both forms via Viper for all CLI flags, prefer legacy
when both are set, and document the dual naming. E2E helpers accept either
form so tests stay aligned with the CLI.
FixesAzure#248
Align markdown tables for markdownlint, switch remaining examples (including the quickstart) to preferred snake_case env names, and merge e2e tfpath assignments that staticcheck flagged after the mpfEnv helper change.
@Abhineshhh

Copy link
Copy Markdown
Author

Mani Bindra (@maniSbindra) Thanks for the review. Rebased onto latest main and fixed the failing lint checks:

Also updated the remaining docs examples, including docs/installation-and-quickstart.md, to the preferred snake_case names (legacy names still work).

Update display-options and release-process samples to the preferred MPF_*_ID / MPF_TF_PATH / MPF_BICEP_EXEC_PATH names. Legacy concatenated names remain documented in the flag reference.
@maniSbindra

Copy link
Copy Markdown
Contributor

Thanks Abhinesh Jha (@Abhineshhh) for addressing the feedback and fixing the checks. The PR now looks good to merge.

We’ll hold the merge briefly to coordinate it with the next MPF release. The updated documentation uses snake_case environment variables, which the currently released binary does not yet support. This avoids users following the new examples before a compatible binary is available.

No further changes are required from you.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Abhineshhh@maniSbindra