Skip to content

Widen installation_history sql_server_versioncolumn #716

Open
jakemorgangit wants to merge 7 commits intoerikdarlingdata:devfrom
jakemorgangit:main
Open

Widen installation_history sql_server_versioncolumn #716
jakemorgangit wants to merge 7 commits intoerikdarlingdata:devfrom
jakemorgangit:main

Conversation

@jakemorgangit
Copy link

Increase sql_server_version to nvarchar(512) to prevent truncation of long SQL Server version strings

What does this PR do?

Fixes - #712

Updates config.installation_history to increase sql_server_version from nvarchar(255) to nvarchar(512).

Some SQL Server version and edition strings can exceed 255 characters. This change prevents truncation and ensures full version information is retained.

Which component(s) does this affect?

  • Full Dashboard
  • Lite Dashboard
  • Lite Tests
  • SQL collection scripts
  • CLI Installer
  • GUI Installer
  • Documentation

How was this tested?

Tested against SQL Server 2019.

Steps:

  1. Applied updated schema via GUI installer and CLI installer script
  2. Inserted values using @@Version and SERVERPROPERTY('Edition')
  3. Verified no truncation occurs for long version strings
  4. Confirmed existing data migrates successfully during table recreation

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • I have tested my changes against at least one SQL Server version
  • I have not introduced any hardcoded credentials or server names

Copy link
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this — the upgrade script and schema change are solid. A couple of things to address before merge:

1. Remove the ALTER TABLE block from 01_install_database.sql (lines 644–672)

This project's convention is that schema changes for existing installations go in upgrades/ scripts only, not in the install script. Your CREATE TABLE on line 623 already has nvarchar(512), so new installs are covered. The upgrade script at upgrades/2.4.1-to-2.4.2/ handles existing installs. The ALTER TABLE block in the install script is redundant and goes against the project pattern — please remove it.

2. Branch hygiene

The PR head ref is main, which means this was committed directly to your fork's main branch. That pulls in unrelated upstream merge commits (there are 4 in the commit list). For future PRs, creating a feature branch (e.g. fix/widen-version-column) keeps the history clean. For this one, we can squash-merge, but a rebase onto a feature branch would be ideal.

3. Minor: commit message vs. actual change

The first commit message says "Increase sql_server_version and sql_server_edition to nvarchar(512)" but the diff only widens sql_server_version. sql_server_edition stays at nvarchar(255). That's probably fine (edition strings are short), but the commit message is misleading.

Everything else looks good — the upgrade script is properly idempotent with correct SET options, the upgrade.txt is in place, and the test helper is updated. Just need item #1 fixed and this is ready to go.

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.

2 participants