dsccommunity / SqlServerDsc

This module contains DSC resources for deployment and configuration of Microsoft SQL Server.
MIT License
360 stars 225 forks source link

BREAKING CHANGE: SqlScript, SqlScriptQuery - Variable as key to allow reuse of a script with different variables #2042

Closed Borgquite closed 3 months ago

Borgquite commented 3 months ago

Pull Request (PR) description

Add 'Variable' as a key to SqlScript, SqlScriptQuery resources to allow reuse of a script with different variables

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94%. Comparing base (c5e7fe7) to head (372251f). Report is 4 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042/graphs/tree.svg?width=650&height=150&src=pr&token=2L5l2Zcoqd&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity)](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) ```diff @@ Coverage Diff @@ ## main #2042 +/- ## ==================================== Coverage 94% 94% ==================================== Files 94 94 Lines 7920 7920 ==================================== Hits 7490 7490 Misses 430 430 ``` | [Flag](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | `94% <ø> (ø)` | | | [Files](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [...urce/DSCResources/DSC\_SqlScript/DSC\_SqlScript.psm1](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042?src=pr&el=tree&filepath=source%2FDSCResources%2FDSC_SqlScript%2FDSC_SqlScript.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfU3FsU2NyaXB0L0RTQ19TcWxTY3JpcHQucHNtMQ==) | `100% <ø> (ø)` | | | [...sources/DSC\_SqlScriptQuery/DSC\_SqlScriptQuery.psm1](https://app.codecov.io/gh/dsccommunity/SqlServerDsc/pull/2042?src=pr&el=tree&filepath=source%2FDSCResources%2FDSC_SqlScriptQuery%2FDSC_SqlScriptQuery.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfU3FsU2NyaXB0UXVlcnkvRFNDX1NxbFNjcmlwdFF1ZXJ5LnBzbTE=) | `100% <ø> (ø)` | |
Borgquite commented 3 months ago

I'm having a go at this, but at the moment I'm struggling to get HQRM tests to pass. Is it possible that it's not permitted to use a String[] as a Key variable (quite likely?)

If that's the case, perhaps what is needed is a new Key variable called something like 'Id' or 'Name' - and the existing Keys (InstanceName, SetFilePath, GetFilePath, TestFilePath) could be removed as Keys. That might remove some of the compile-time protections against idempotency but would open up the opportunity to reuse a script file multiple times with just the variables needing changing.

dan-hughes commented 3 months ago

@Borgquite I believe you are correct, Key properties need to be 'simple'. I can't find where it's documented though.

Why would not adding another Key property like you mention not work?

Would there be a situation where the database name would be a Key property but used as a variable? Or would the same scripts be reused against the same database?

Borgquite commented 3 months ago

As discussed on the #dsc channel, I'll redo this patch with a new Key parameter which should allow us to do this. It will be a breaking change but we've agreed that is pretty unavoidable in the circumstances.

Borgquite commented 3 months ago

Opened new pull request at #2043