Skip to content

feat: Add Windows-specific default config file paths#5073

Open
coatico wants to merge 3 commits intoprometheus:mainfrom
coatico:main
Open

feat: Add Windows-specific default config file paths#5073
coatico wants to merge 3 commits intoprometheus:mainfrom
coatico:main

Conversation

@coatico
Copy link
Copy Markdown

@coatico coatico commented Mar 5, 2026

Pull Request Checklist

Please check all the applicable boxes.

  • Please list all open issue(s) discussed with maintainers related to this change
  • Is this a new Receiver integration?
  • Is this a bugfix?
    • I have added tests that can reproduce the bug which pass with this bugfix applied
  • I have added/updated the required documentation
  • I have signed-off my commits
  • I will follow best practices for contributing to this project

Which user-facing changes does this PR introduce?

NONE

Summary by CodeRabbit

  • Chores

    • Configuration lookup now uses platform-standard locations. On Windows the app respects per-user and system config directories; on non-Windows it follows XDG/Unix conventions with per-user and system candidate locations.
  • Tests

    • Added platform-specific tests to validate the resolved per-user and system config file candidates across Windows and non-Windows environments.

@coatico coatico force-pushed the main branch 7 times, most recently from 55a0855 to 7bdf6c7 Compare March 10, 2026 11:57
@TheMeier
Copy link
Copy Markdown
Contributor

tried to test locally:

$ make
>> checking code style
gofmt checking failed!
diff cli/root_others.go.orig cli/root_others.go
--- cli/root_others.go.orig
+++ cli/root_others.go
@@ -25,12 +25,12 @@
 	if err != nil {
 		userConfigDir = filepath.Join(os.ExpandEnv("$HOME"), ".config")
 	}
-	
+
 	systemConfigDir := "/etc"
 	if envConfigDir := os.Getenv("XDG_CONFIG_DIRS"); envConfigDir != "" {
 		systemConfigDir = filepath.SplitList(envConfigDir)[0]
 	}
-	
+
 	return []string{
 		filepath.Join(userConfigDir, "amtool", "config.yml"),
 		filepath.Join(systemConfigDir, "amtool", "config.yml"),

Please ensure you are using go version go1.25.7 X:nodwarf5 linux/amd64 for formatting code.
make: *** [Makefile.common:154: common-style] Error 1

@coatico coatico force-pushed the main branch 5 times, most recently from fcdab67 to bfdd117 Compare March 13, 2026 09:06
@coatico coatico requested a review from TheMeier March 13, 2026 09:09
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 23, 2026

📝 Walkthrough

Walkthrough

This PR implements OS-specific default configuration file paths for amtool. It replaces hard-coded Unix paths with platform-aware logic: non-Windows systems use $HOME/.config and /etc (with XDG support), while Windows uses %APPDATA% and %ProgramData%.

Changes

Cohort / File(s) Summary
Root CLI
cli/root.go
configFiles now initialized via defaultConfigFiles(); CLI help text updated to describe OS-dependent default config locations (non-Windows vs Windows).
Non-Windows implementation & tests
cli/root_others.go, cli/root_others_test.go
Added defaultConfigFiles() (build tag !windows) that resolves user config from os.UserConfigDir() with $HOME/.config fallback, system config from /etc or first XDG_CONFIG_DIRS entry. Added tests covering XDG and fallback scenarios.
Windows implementation & tests
cli/root_windows.go, cli/root_windows_test.go
Added defaultConfigFiles() (build tag windows) that resolves user config via os.UserConfigDir() or %APPDATA% fallback and system config via %ProgramData%. Added Windows-specific test validating both paths.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hopped through envs both near and far,
From XDG hills to a Windows star.
AppData, ProgramData, HOME and /etc,
I stitched their paths with a tiny tech check.
Now configs meet, wherever users are.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description includes all critical checklist items: linked issue (#3331), test confirmation, documentation updates, signed commits, and contribution best practices compliance. Release notes section completed.
Linked Issues check ✅ Passed The code changes fully address issue #3331 by introducing platform-specific config file path resolution: non-Windows paths use $HOME/.config/amtool/config.yml and /etc/amtool/config.yml, while Windows paths use %APPDATA%\amtool\config.yml and %ProgramData%\amtool\config.yml for system-wide access.
Out of Scope Changes check ✅ Passed All changes are scoped to implementing Windows-specific default config paths and corresponding tests; no unrelated modifications detected in cli/root.go, cli/root_others.go, cli/root_windows.go, and their test files.
Title check ✅ Passed The title accurately describes the main objective of the PR: adding Windows-specific default config file paths. The changes include platform-specific implementations for both Windows and non-Windows systems with corresponding tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
cli/root_others_test.go (1)

24-78: LGTM - Tests cover key scenarios for non-Windows paths.

Both test functions properly validate the default behavior and XDG_CONFIG_DIRS override. Using t.Setenv ensures clean environment restoration.

Consider adding a test case where XDG_CONFIG_HOME is set to a non-empty value to verify os.UserConfigDir() respects it:

💡 Optional test case for XDG_CONFIG_HOME
func TestDefaultConfigFilesOthersWithXDGConfigHome(t *testing.T) {
	home := t.TempDir()
	customConfigHome := t.TempDir()
	t.Setenv("HOME", home)
	t.Setenv("XDG_CONFIG_HOME", customConfigHome)
	t.Setenv("XDG_CONFIG_DIRS", "")

	files := defaultConfigFiles()

	if len(files) != 2 {
		t.Fatalf("expected 2 config file paths, got %d", len(files))
	}

	expectedUser := filepath.Join(customConfigHome, "amtool", "config.yml")
	if files[0] != expectedUser {
		t.Errorf("expected user config path %q, got %q", expectedUser, files[0])
	}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/root_others_test.go` around lines 24 - 78, Add a test to cover the case
when XDG_CONFIG_HOME is set: create a new test (e.g.,
TestDefaultConfigFilesOthersWithXDGConfigHome) that uses t.TempDir() for HOME
and another temp dir for custom XDG_CONFIG_HOME, set t.Setenv("HOME", home) and
t.Setenv("XDG_CONFIG_HOME", customConfigHome") and t.Setenv("XDG_CONFIG_DIRS",
""), call defaultConfigFiles(), assert len(files)==2 and that files[0] equals
filepath.Join(customConfigHome, "amtool", "config.yml"); reference the existing
tests TestDefaultConfigFilesOthers and
TestDefaultConfigFilesOthersWithXDGConfigDirs and the function
defaultConfigFiles to locate where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cli/root_others_test.go`:
- Around line 24-78: Add a test to cover the case when XDG_CONFIG_HOME is set:
create a new test (e.g., TestDefaultConfigFilesOthersWithXDGConfigHome) that
uses t.TempDir() for HOME and another temp dir for custom XDG_CONFIG_HOME, set
t.Setenv("HOME", home) and t.Setenv("XDG_CONFIG_HOME", customConfigHome") and
t.Setenv("XDG_CONFIG_DIRS", ""), call defaultConfigFiles(), assert len(files)==2
and that files[0] equals filepath.Join(customConfigHome, "amtool",
"config.yml"); reference the existing tests TestDefaultConfigFilesOthers and
TestDefaultConfigFilesOthersWithXDGConfigDirs and the function
defaultConfigFiles to locate where to add this case.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: facfa22c-eae6-48ca-ac6f-9f6385f1097e

📥 Commits

Reviewing files that changed from the base of the PR and between 7aa1b9f and 1ab067f.

📒 Files selected for processing (5)
  • cli/root.go
  • cli/root_others.go
  • cli/root_others_test.go
  • cli/root_windows.go
  • cli/root_windows_test.go

Signed-off-by: VAN LINT <9860220+coatico@users.noreply.github.com>
@coatico coatico changed the title Add Windows-specific default config file paths feat: Add Windows-specific default config file paths Mar 25, 2026
@coatico coatico requested a review from a team as a code owner March 30, 2026 12:25
Sign up for free to 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.

amtool: /etc/amtool.yml or $home/.config/amtool/ on windows server

2 participants