Skip to content

fix: reject non-YAML extensions for --sqlconfig flag#684

Open
Copilot wants to merge 1 commit intomainfrom
copilot/add-documentation-for-config-types
Open

fix: reject non-YAML extensions for --sqlconfig flag#684
Copilot wants to merge 1 commit intomainfrom
copilot/add-documentation-for-config-types

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 31, 2026

Problem

--sqlconfig only supports YAML (viper.SetConfigType("yaml") is hardcoded, Save() uses yaml.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

SetFileName accepted any path without checking the extension. No docs mentioned the YAML-only constraint.

Solution

  1. Validate the file extension in config.SetFileName: if an extension is present, it must be .yaml or .yml. Extensionless files (like the default ~/.sqlcmd/sqlconfig) are allowed.
  2. Document the YAML requirement in the README and --sqlconfig help text.
  3. Regenerate catalog.go and locale files via go generate.

Changes

File Change
internal/config/config.go Add extension validation in SetFileName
internal/config/config_test.go Add TestNegConfig_SetFileNameBadExtension covering .json, .toml, .yaml, .yml
README.md Add Custom Configuration Files section documenting YAML format and extension requirements
cmd/modern/root.go Update --sqlconfig help text to YAML configuration file (.yaml or .yml extension)
internal/translations/catalog.go Regenerated via go generate
internal/translations/locales/*/out.gotext.json Regenerated locale entries for new help string

Testing

  • go build ./cmd/modern passes
  • go vet ./... passes
  • go test ./internal/config/ passes (including new extension validation test)

Fixes #690. Fixes #597.

Copilot AI changed the title [WIP] Add documentation for supported config types Document supported config file extensions for --sqlconfig flag Jan 31, 2026
Copilot AI requested a review from dlevy-msft-sql January 31, 2026 20:00
@dlevy-msft-sql dlevy-msft-sql added bug Something isn't working Size: S Small issue (less than one week effort) needs-work labels Feb 1, 2026
Copy link
Copy Markdown

Copilot AI left a 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 --sqlconfig flag 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

Comment thread internal/translations/locales/de-DE/out.gotext.json
Copy link
Copy Markdown

Copilot AI left a 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.

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread internal/translations/locales/zh-CN/out.gotext.json
@dlevy-msft-sql
Copy link
Copy Markdown
Contributor

@copilot address the review comments

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 1, 2026

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:

  • does
    • Triggering command: /tmp/go-build2543538777/b536/install.test /tmp/go-build2543538777/b536/install.test -test.testlogfile=/tmp/go-build2543538777/b536/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -I /tmp/net_C387458-ifaceassert x64/bin/go . oot}} {{.Goroot}-atomic --64 x64/bin/go -w -Wno-error -o/tmp/os_user_C-ifaceassert ux_amd64/vet -c oot}} {{.Goroot}-atomic /tmp/os_user_C18-bool ux_amd64/vet (dns block)
  • nonexistent.database.windows.net
    • Triggering command: /tmp/sqlcmd-e2e-847287666/sqlcmd /tmp/sqlcmd-e2e-847287666/sqlcmd -G -S nonexistent.database.windows.net -d testdb -compiler=gc ndor/bin/bash -installsuffix= oot}} {{.Goroot}-atomic -- hentication-libr-buildtags list�� -e store missing tr-ifaceassert ux_amd64/vet -installsuffix= mportPath}} {{.R-atomic -- ux_amd64/vet (dns block)
  • nonexistent.server
    • Triggering command: /tmp/sqlcmd-e2e-847287666/sqlcmd /tmp/sqlcmd-e2e-847287666/sqlcmd -S nonexistent.server -tests /tmp/go-build2543538777/b321/vet.cfg -e -compiler=gc ache/go/1.24.12/-nilfunc -installsuffix= oot}} {{.Goroot}-atomic -- nal/flags list�� -e store missing tr-ifaceassert ux_amd64/vet -installsuffix= mportPath}} {{.R-atomic -- ux_amd64/vet (dns block)
    • Triggering command: `/tmp/sqlcmd-e2e-847287666/sqlcmd /tmp/sqlcmd-e2e-847287666/sqlcmd -C -S nonexistent.server /tmp/go-build2543538777/b436/vet.cfg -I /tmp/os_user_C18-ifaceassert ache/go/1.24.12/-nilfunc . oot}}
      {{.Goroot}-test.testlogfile=/tmp/go-build3625500898/b001/testlog.txt --64 /v1.20.0 -W -I store missing translations

Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.norep-o ux_amd64/vet . mportPath}}
{{.R-atomic --64 ux_amd64/vet` (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copy link
Copy Markdown

Copilot AI left a 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.

Comment thread README.md Outdated
Comment thread cmd/modern/root.go Outdated
Comment thread README.md Outdated
@dlevy-msft-sql
Copy link
Copy Markdown
Contributor

@copilot fix the items in the review

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 1, 2026

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:

  • does
    • Triggering command: /tmp/go-build3563136012/b536/install.test /tmp/go-build3563136012/b536/install.test -test.testlogfile=/tmp/go-build3563136012/b536/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ndLinux.slice -mtune=generic x64/bin/go (dns block)
  • nonexistent.database.windows.net
    • Triggering command: /tmp/sqlcmd-e2e-3521548478/sqlcmd /tmp/sqlcmd-e2e-3521548478/sqlcmd -G -S nonexistent.database.windows.net -d testdb (dns block)
  • nonexistent.server
    • Triggering command: /tmp/sqlcmd-e2e-3521548478/sqlcmd /tmp/sqlcmd-e2e-3521548478/sqlcmd -S nonexistent.server -tests /tmp/go-build3563136012/b455/vet.cfg ndLinux.slice 02160869 x64/bin/go sVt9gHpKW oot}} {{.Goroot}-atomic cal/bin/git x64/bin/go 0216�� log.showsignatur-errorsas log ux_amd64/compile-nilfunc -n1 oot}} {{.Goroot}-atomic ff25b1343cfc ux_amd64/compile-buildtags (dns block)
    • Triggering command: /tmp/sqlcmd-e2e-3521548478/sqlcmd /tmp/sqlcmd-e2e-3521548478/sqlcmd -C -S nonexistent.server /tmp/go-build3563136012/b417/vet.cfg --noprofile /home/REDACTED/go/pkg/mod/golang.org/x/text@v0.33.-f={{.Dir}} {{.ImportPath}} {{.R/tmp/go-build3563136012/b536/importcfg x64/bin/go (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Document supported config file extensions for --sqlconfig flag Document that --sqlconfig flag only supports YAML format Feb 1, 2026
Copy link
Copy Markdown

Copilot AI left a 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.

Copy link
Copy Markdown
Contributor

@dlevy-msft-sql dlevy-msft-sql left a comment

Choose a reason for hiding this comment

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

PR #684 Review: Document that --sqlconfig flag only supports YAML format

What the PR Does

  1. Updates --sqlconfig flag help text in cmd/modern/root.go:

    • Before: "configuration file"
    • After: "configuration file (YAML format, use .yaml or .yml extension)"
  2. Adds README documentation explaining custom config file usage with examples

  3. Updates translation files with the new string (+ fixes unrelated JSON escape changes for >= and <command>)

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

@dlevy-msft-sql
Copy link
Copy Markdown
Contributor

@copilot address the feedback in my review

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 1, 2026

@copilot address the feedback in my review

Addressed in commit 1440de1:

  1. README clarification: Changed wording from "should use" to "use" and explicitly noted the default file is "an exception" to avoid confusion about extensions.

  2. Unrelated translation changes: Fixed JSON escape sequences - restored \u003e and \u003c escaping for > and < characters that were accidentally unescaped when manually adding the new translation entry.

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:

  • does
    • Triggering command: /tmp/go-build609210551/b536/install.test /tmp/go-build609210551/b536/install.test -test.testlogfile=/tmp/go-build609210551/b536/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a lliptic/elliptic.go ux_amd64/compile (dns block)
  • nonexistent.database.windows.net
    • Triggering command: /tmp/sqlcmd-e2e-389345408/sqlcmd /tmp/sqlcmd-e2e-389345408/sqlcmd -G -S nonexistent.database.windows.net -d testdb (dns block)
  • nonexistent.server
    • Triggering command: /tmp/sqlcmd-e2e-389345408/sqlcmd /tmp/sqlcmd-e2e-389345408/sqlcmd -S nonexistent.server -tests /tmp/go-build609210551/b462/vet.cfg 1627803/b056/_pkg_.a (dns block)
    • Triggering command: /tmp/sqlcmd-e2e-389345408/sqlcmd /tmp/sqlcmd-e2e-389345408/sqlcmd -C -S nonexistent.server /tmp/go-build609210551/b496/vet.cfg .10.0/internal/time.go .10.0/internal/timeformattype_st--gdwarf2 x64/pkg/tool/linux_amd64/compile--64 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@dlevy-msft-sql
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 2, 2026

@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 warn...

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:
Currently, when users specify a config file with an unsupported extension (e.g., --sqlconfig myfile.txt), they receive a confusing Viper error: "Unsupported Config Type 'txt'".

Proposed Enhancement:
Add validation in internal/config/viper.go to check the file extension before passing to Viper and provide a clearer error message such as:

Error: Configuration files must use YAML format with .yaml or .yml extension.
The file 'myfile.txt' has an unsupported extension '.txt'.

Related:

Implementation Notes:

  • Check file extension in configureViper() or SetFileName() function
  • Allow .yaml, .yml, and no extension (for default ~/.sqlcmd/sqlconfig)
  • Provide helpful error message pointing users to documentation

Copy link
Copy Markdown

Copilot AI left a 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.

@dlevy-msft-sql dlevy-msft-sql force-pushed the copilot/add-documentation-for-config-types branch from 0066869 to 77fffe6 Compare April 16, 2026 20:11
@dlevy-msft-sql dlevy-msft-sql changed the title Document that --sqlconfig flag only supports YAML format docs: document --sqlconfig YAML-only format requirement Apr 16, 2026
@dlevy-msft-sql dlevy-msft-sql changed the title docs: document --sqlconfig YAML-only format requirement fix: reject non-YAML extensions for --sqlconfig flag Apr 16, 2026
@dlevy-msft-sql dlevy-msft-sql force-pushed the copilot/add-documentation-for-config-types branch from 77fffe6 to 32948a1 Compare April 16, 2026 20:21
@dlevy-msft-sql dlevy-msft-sql force-pushed the copilot/add-documentation-for-config-types branch from 32948a1 to 3d689dd Compare April 16, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Size: S Small issue (less than one week effort)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add validation/warning for unsupported config file extensions Add documentation/help about supported config types

3 participants