Skip to content
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

SqlMemory: Allow setting of dynamic value for min server memory #1397 #1695

Merged
merged 13 commits into from
Apr 17, 2021

Conversation

TrisBits
Copy link
Contributor

@TrisBits TrisBits commented Mar 6, 2021

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

johlju and others added 12 commits February 22, 2021 15:31
…sccommunity#1692)

- SqlSetup
  - The helper function `Connect-SqlAnalysis` was using `LoadWithPartial()`
    to load the assembly _Microsoft.AnalysisServices_. On a node where multiple
    instances with different versions of SQL Server (regardless of features)
    is installed, this will result in the first assembly found in the
    GAC will be loaded into the session, not taking versions into account.
    This can result in an assembly version being loaded that is not compatible
    with the version of SQL Server it was meant to be used with.
    A new method of loading the assembly _Microsoft.AnalysisServices_ was
    introduced under a feature flag; `'AnalysisServicesConnection'`.
    This new functionality depends on the [SqlServer](https://www.powershellgallery.com/packages/SqlServer)
    module, and must be present on the node. The [SqlServer](https://www.powershellgallery.com/packages/SqlServer)
    module can be installed on the node by leveraging the new DSC resource
    `PSModule` in the [PowerShellGet](https://www.powershellgallery.com/packages/PowerShellGet/2.1.2)
    module (v2.1.2 and higher). This new method does not work with the
    SQLPS module due to the SQLPS module does not load the correct assembly,
    while [SqlServer](https://www.powershellgallery.com/packages/SqlServer)
    module (v21.1.18080 and above) does. The new functionality is used
    when the parameter `FeatureFlag` is set to `'AnalysisServicesConnection'`.
    This functionality will be the default in a future breaking release.
  - Under a feature flag; `'AnalysisServicesConnection'`. The detection of
    a successful connection to the SQL Server Analysis Services has also been
    changed. Now it actually evaluates the property `Connected` of the returned
    `Microsoft.AnalysisServices.Server` object. The new functionality is used
    when the parameter `FeatureFlag` is set to `'AnalysisServicesConnection'`.
    This functionality will be the default in a future breaking release.
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #1695 (b27cf22) into main (75a8161) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1695   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6285    6325   +40     
====================================
+ Hits       6129    6169   +40     
  Misses      156     156           
Flag Coverage Δ
unit 97% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...urce/DSCResources/DSC_SqlMemory/DSC_SqlMemory.psm1 99% <100%> (+<1%) ⬆️

@johlju johlju added the needs review The pull request needs a code review. label Mar 7, 2021
@johlju
Copy link
Member

johlju commented Mar 10, 2021

I will review this as soon as I can. There is a lot at the day job and also have a backlog of reviews. But this is on the todo list. 🙂

@TrisBits
Copy link
Contributor Author

@johlju Not to worry, I completely understand. Thank you for the update and I look forward to your review and feedback :)

@johlju
Copy link
Member

johlju commented Mar 30, 2021

This is still on the todo list. Just a lot of work getting DnsServerDsc out the door. I will swing back here soon. I will try to get to the backlog of all reviews during Easter.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TrisBits)

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TrisBits)

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed needs review The pull request needs a code review. labels Apr 17, 2021
@johlju
Copy link
Member

johlju commented Apr 17, 2021

@TrisBits sorry it took so long - been working on tooling and other modules for a while. This looks great! Awesome work on the tests! 😃

@johlju johlju changed the title SqlMemory: Allow setting of dynamic value for min server memory. #1397 SqlMemory: Allow setting of dynamic value for min server memory #1397 Apr 17, 2021
@TrisBits
Copy link
Contributor Author

@johlju Thank you for the code review and feedback. Not to worry about the delay, you have a lot on your plate.

@johlju johlju merged commit 49f78a8 into dsccommunity:main Apr 17, 2021
@johlju johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Apr 17, 2021
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.

SqlMemory: Allow setting of dynamic value for min server memory.
2 participants