Skip to content

fix(config): handle directory paths for --config and HARBOR_CLI_CONFIG#994

Open
Jay2006sawant wants to merge 1 commit into
goharbor:mainfrom
Jay2006sawant:fix/config-directory-path-validation
Open

fix(config): handle directory paths for --config and HARBOR_CLI_CONFIG#994
Jay2006sawant wants to merge 1 commit into
goharbor:mainfrom
Jay2006sawant:fix/config-directory-path-validation

Conversation

@Jay2006sawant

Copy link
Copy Markdown

Description

Briefly describe what this pull request does and why the change is needed.

When --config or HARBOR_CLI_CONFIG pointed to a directory (as documented in the README), Harbor CLI persisted that directory path into data.yaml before validating it was a file. This caused a fatal error on every subsequent run until data.yaml was manually repaired.

This PR resolves directory paths to <dir>/config.yaml and ensures the config file is validated before updating data.yaml.

Type of Change

Please select the relevant type.

  • Bug fix
  • New feature
  • Refactor
  • Documentation update
  • Chore / maintenance

Changes

  • Add normalizeConfigPath to resolve directory paths to config.yaml for --config and HARBOR_CLI_CONFIG
  • Reorder InitConfig to call EnsureConfigFileExists before ApplyDataFile, preventing invalid paths from being persisted
  • Add unit tests for directory-based config paths (Test_Config_EnvVar_Directory, Test_Config_Flag_Directory)

Test plan

  • go test ./pkg/utils/... -run Test_Config
  • harbor -c ~/.config/harbor-cli version — exits 0, data.yaml stores file path
  • HARBOR_CLI_CONFIG=$HOME/.config/harbor-cli harbor version — exits 0
  • Subsequent harbor version runs work without manual data.yaml repair

Fixes goharbor#993

Signed-off-by: Jay2006sawant <jay242902@gmail.com>
@Jay2006sawant

Copy link
Copy Markdown
Author

@Vad1mo PTAL!

@qcserestipy qcserestipy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for catching this! This can be simplified by just checking for file or dir in DetermineConfigPath. Added tests and normalization function can therefore be removed/adapted.

Comment thread pkg/utils/config.go
return "", fmt.Errorf("failed to resolve absolute path for config file: %w", err)
}
return harborConfigPath, nil
return normalizeConfigPath(cfgFile)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the cli should return an error instead of automagically appending a config.yaml suffix. it should be enough to check here whether the path is a file or dir. if dir fail

@qcserestipy qcserestipy added bug Something isn't working status/in-progress Work on this issue has started or a linked pull request is actively being developed. Changes Requesed feedback that must be addressed before merging. labels Jun 18, 2026
@qcserestipy

Copy link
Copy Markdown
Collaborator

@Jay2006sawant Please check out the suggested changes

@qcserestipy qcserestipy added the status/needs-feedback Waiting for input from the issue author or reporter. May be closed after 7 days. label Jun 30, 2026
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 9.18%. Comparing base (60ad0bd) to head (467b2a0).
⚠️ Report is 188 commits behind head on main.

Files with missing lines Patch % Lines
pkg/utils/config.go 63.63% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             main    #994      +/-   ##
=========================================
- Coverage   10.99%   9.18%   -1.81%     
=========================================
  Files         173     321     +148     
  Lines        8671   16083    +7412     
=========================================
+ Hits          953    1478     +525     
- Misses       7612   14472    +6860     
- Partials      106     133      +27     

☔ 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.

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

Labels

bug Something isn't working Changes Requesed feedback that must be addressed before merging. status/in-progress Work on this issue has started or a linked pull request is actively being developed. status/needs-feedback Waiting for input from the issue author or reporter. May be closed after 7 days.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: Config directory path via --config or HARBOR_CLI_CONFIG corrupts data.yaml and bricks CLI

2 participants