-
Notifications
You must be signed in to change notification settings - Fork 82
Document that --sqlconfig flag only supports YAML format #684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
|
@copilot address the review comments |
…tions Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
|
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}}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
|
@copilot fix the items in the review |
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
|
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Summary
This PR addresses issue #597 by documenting that sqlcmd's
--sqlconfigflag only supports YAML format. Initial investigation revealed the underlying Viper library supports multiple formats, but the sqlcmd implementation hardcodes YAML format.Root Cause
The implementation in
internal/config/viper.go:viper.SetConfigType("yaml")- hardcodes YAML for readingyaml.Marshal(&config)- always marshals as YAML for writingThis means only YAML-formatted files work correctly for both read and write operations, regardless of file extension.
Changes Made
cmd/modern/root.go
--sqlconfigflag help text to: "configuration file (YAML format, use .yaml or .yml extension)"README.md
Translation files
Testing
The documentation now accurately reflects the actual implementation behavior.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.