fix: reject non-YAML extensions for --sqlconfig flag#684
fix: reject non-YAML extensions for --sqlconfig flag#684
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #597 by documenting the supported configuration file extensions for the --sqlconfig flag. Users were encountering "Unsupported Config Type" errors when using non-standard extensions like .sqlconfig without knowing which extensions are actually supported by Viper.
Changes:
- Updated
--sqlconfigflag help text to list supported extensions inline - Added comprehensive "Custom Configuration Files" section to README.md with examples
- Regenerated translation catalogs for new localizable strings
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/modern/root.go | Added inline documentation of supported extensions (.yaml, .yml, .json, .toml, .ini, .env, .properties) to the --sqlconfig flag help text |
| README.md | Added new "Custom Configuration Files" section listing all supported extensions with format names, including variants and less common formats, plus usage examples and clarifying note about unsupported extensions |
| internal/translations/locales/*/out.gotext.json | Regenerated translation catalogs with new help text string; non-English locales have empty translations awaiting human translation |
|
@copilot address the review comments |
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.norep-o ux_amd64/vet . mportPath}}
|
|
@copilot fix the items in the review |
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
dlevy-msft-sql
left a comment
There was a problem hiding this comment.
PR #684 Review: Document that --sqlconfig flag only supports YAML format
What the PR Does
-
Updates
--sqlconfigflag help text incmd/modern/root.go:- Before:
"configuration file" - After:
"configuration file (YAML format, use .yaml or .yml extension)"
- Before:
-
Adds README documentation explaining custom config file usage with examples
-
Updates translation files with the new string (+ fixes unrelated JSON escape changes for
>=and<command>) -
Fixes missing newlines at EOF in all translation files
Potential Issues to Consider
📋 Minor: The documentation could be clearer
The README states files "should use the .yaml or .yml extension" but the default ~/.sqlcmd/sqlconfig has no extension. This is documented in the Note, but the wording could be confusing.
⚠️ Unrelated changes in translation files
The PR changes \u003e= to >= and \u003ccommand\u003e to <command> in all translation files. While these are cosmetic (JSON escape sequences for >, <), they shouldn't affect functionality, but they inflate the diff and are unrelated to the stated purpose.
✅ Missing newline fixes are fine
Adding the missing newline at EOF is a good housekeeping change.
🤔 Consider: Should the code validate the file extension?
The PR documents the YAML requirement, but if users use an unsupported extension, they'll still get a confusing error. A future enhancement could add validation or a warning.
Verdict
The PR accomplishes its stated goal of documenting the YAML-only limitation. The changes are minimal and correct. The translation file changes are slightly noisy but harmless. Looks good to merge once taken out of draft status.
|
@copilot address the feedback in my review |
Addressed in commit 1440de1:
The diff is now focused on just the intended documentation changes. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot create new issue for The PR documents the YAML requirement, but if users use an unsupported extension, they'll still get a confusing error. A future enhancement could add validation or a warning. |
I don't have the ability to create GitHub issues directly. However, here's the issue content that can be created: Title: Add validation/warning for unsupported config file extensions Description: Proposed Enhancement: Related:
Implementation Notes:
|
0066869 to
77fffe6
Compare
77fffe6 to
32948a1
Compare
32948a1 to
3d689dd
Compare
Problem
--sqlconfigonly supports YAML (viper.SetConfigType("yaml")is hardcoded,Save()usesyaml.Marshal), but passing a file with a non-YAML extension (e.g.foo.json) silently parses it as YAML, producing garbage or a cryptic error. No validation, no documentation.Root Cause
SetFileNameaccepted any path without checking the extension. No docs mentioned the YAML-only constraint.Solution
config.SetFileName: if an extension is present, it must be.yamlor.yml. Extensionless files (like the default~/.sqlcmd/sqlconfig) are allowed.--sqlconfighelp text.catalog.goand locale files viago generate.Changes
internal/config/config.goSetFileNameinternal/config/config_test.goTestNegConfig_SetFileNameBadExtensioncovering.json,.toml,.yaml,.ymlREADME.mdCustom Configuration Filessection documenting YAML format and extension requirementscmd/modern/root.go--sqlconfighelp text toYAML configuration file (.yaml or .yml extension)internal/translations/catalog.gogo generateinternal/translations/locales/*/out.gotext.jsonTesting
go build ./cmd/modernpassesgo vet ./...passesgo test ./internal/config/passes (including new extension validation test)Fixes #690. Fixes #597.