Skip to content

test(config): cover CLI flag precedence - #931

Open
pollychen-lab wants to merge 1 commit into
padok-team:mainfrom
pollychen-lab:test/issue-146-cli-flag-precedence
Open

test(config): cover CLI flag precedence#931
pollychen-lab wants to merge 1 commit into
padok-team:mainfrom
pollychen-lab:test/issue-146-cli-flag-precedence

Conversation

@pollychen-lab

Copy link
Copy Markdown

Summary

Fix configuration precedence so explicitly supplied CLI flags override configuration files while environment variables continue to take priority over CLI flags.

Root cause

The CLI exposes flat flag names such as --addr, but Viper was binding those names directly while the corresponding configuration keys are nested, such as server.addr. Viper's default precedence also places flags above environment variables, contrary to Burrito's documented precedence.

Changes

  • Map supported CLI flag names to their nested configuration keys.
  • Promote explicitly set BURRITO_* environment variables above bound flags.
  • Add tests for config file, CLI flag, and environment variable precedence.
  • Remove the obsolete commented-out flag test.

Validation

  • go test ./internal/burrito/config
  • go test -race ./internal/burrito/config
  • go test ./cmd/... ./internal/burrito/config
  • go vet ./internal/burrito/config ./cmd/...
  • git diff --check

Fixes #146

@github-project-automation github-project-automation Bot moved this to 📋 Backlog in Burrito Jun 29, 2026
@pollychen-lab pollychen-lab changed the title [codex] test(config): cover CLI flag precedence test(config): cover CLI flag precedence Jun 29, 2026
@pollychen-lab
pollychen-lab marked this pull request as ready for review June 29, 2026 17:28

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ece4fc05de

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/burrito/config/config.go Outdated
keyParts := append(parts, tv)
key := strings.Join(keyParts, ".")
envName := "BURRITO_" + strings.ToUpper(strings.Join(keyParts, "_"))
if value, exists := os.LookupEnv(envName); exists {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep empty env vars from overriding config

When a BURRITO_* variable is present but empty, this branch now promotes it into Viper's Set override layer, so it beats both flags and config files. Viper's normal env handling ignores empty values unless AllowEmptyEnv is enabled, so an empty key from something like envFrom can now silently erase a configured value or decode an empty boolean as false; preserve the previous non-empty check before calling Set.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for catching this. I fixed it in f05d957 by only promoting non-empty environment values into Viper's override layer, preserving the previous empty-variable behavior. I also added a regression test confirming that an empty BURRITO_SERVER_ADDR does not override an explicitly set flag. The focused tests, race test, and vet all pass.

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.55%. Comparing base (87503c8) to head (f05d957).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/burrito/config/config.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
+ Coverage   42.39%   42.55%   +0.15%     
==========================================
  Files          97       97              
  Lines        5859     5868       +9     
==========================================
+ Hits         2484     2497      +13     
+ Misses       3160     3157       -3     
+ Partials      215      214       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pollychen-lab
pollychen-lab force-pushed the test/issue-146-cli-flag-precedence branch from f05d957 to 8a89670 Compare July 13, 2026 02:19
@pollychen-lab
pollychen-lab force-pushed the test/issue-146-cli-flag-precedence branch from 8a89670 to 5f84233 Compare July 13, 2026 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: 📋 Backlog

Development

Successfully merging this pull request may close these issues.

[Test] CLI flags overrides should be tested

2 participants