SqlAG/SqlAGDatabase/SqlAGReplica - Updating examples to include enable HADR option

[wsmelton]

Pull Request (PR) description

Updating examples to include enabling the HADR option for SQL Server.

This Pull Request (PR) fixes the following issues

Fixes #1182

Task list

• Added an entry under the Unreleased section in the CHANGELOG.md? Entry
  should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md?
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help?
• Comment-based help added/updated?
• Localization strings added/updated in all localization files as appropriate?
• Examples appropriately added/updated?
• Unit tests added/updated?
  See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible)?
  See DSC Resource Testing Guidelines.
• New/changed code adheres to
  DSC Resource Style Guidelines
  and Best Practices?

---

This change is 

[johlju]

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @wsmelton)

---

CHANGELOG.md, line 14 at r1 (raw file):

    ([issue #1197](https://github.com/PowerShell/SqlServerDsc/issues/1197)).
- Changes to SqlAg, SqlAGDatabase, and SqlAGReplica examples
  - Included configuration for SqlAlwaysOnService to enable HADR on each node to avoid confusion.

I think this lines fails in the tests.
https://ci.appveyor.com/project/PowerShell/sqlserverdsc/build/9.0.424.0?fullLog=true#L2133

---

Examples/Resources/SqlAG/1-CreateAvailabilityGroup.ps1, line 66 at r1 (raw file):

        # Ensure the HADR option is enabled for the instance
        SqlAlwaysOnService EnableHADR {

We should have the open brace on a separate row.

---

Examples/Resources/SqlAG/1-CreateAvailabilityGroup.ps1, line 70 at r1 (raw file):

            InstanceName = $Node.InstanceName
            ServerName   = $Node.NodeName
            PsDscRunAsCredential = $SqlAdministratorCredential

Can we align the equal signs (as the other examples)? (If using VSCode, hit Alt+Shift+F to auto-format)

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 83 at r1 (raw file):

        }

        SqlAlwaysOnService EnableHADR {

We should have the open brace on a separate row.

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 87 at r1 (raw file):

            InstanceName = $Node.InstanceName
            ServerName   = $Node.NodeName
            PsDscRunAsCredential = $SqlAdministratorCredential

Can we align the equal signs (as the other examples)? (If using VSCode, hit Alt+Shift+F to auto-format)

---

Examples/Resources/SqlAGDatabase/1-AddDatabaseToAvailabilityGroup.ps1, line 79 at r1 (raw file):

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

---

Examples/Resources/SqlAGDatabase/3-MatchDefinedDatabaseInAvailabilityGroup.ps1, line 74 at r1 (raw file):

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

---

Examples/Resources/SqlAGReplica/1-CreateAvailabilityGroupReplica.ps1, line 80 at r1 (raw file):

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

[johlju]

@wsmelton Thank you for fixing this! 😃 Just minor comments.

[codecov-io]

Codecov Report

Merging #1208 into dev will not change coverage.
The diff coverage is n/a.

@@         Coverage Diff          @@
##            dev   #1208   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        33      33           
  Lines      4044    4044           
====================================
  Hits       3957    3957           
  Misses       87      87

[wsmelton]

Curious to know why the built in review features on GitHub are not used instead of a 3rd party tool? I have not submitted PRs to other major projects within MS so unsure if this is common tool or not. It is interesting.

The fact yall also validate a line is only 80 characters wide for markdown is just ridiculous to me as well. This is not coded language and has built in ability to wrap the line for viewing. But is what it is.

Anyway, InstanceName is the correct property name:

[wsmelton]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @johlju and @wsmelton)

---

Examples/Resources/SqlAG/1-CreateAvailabilityGroup.ps1, line 66 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have the open brace on a separate row.

Done.

---

Examples/Resources/SqlAG/1-CreateAvailabilityGroup.ps1, line 70 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we align the equal signs (as the other examples)? (If using VSCode, hit Alt+Shift+F to auto-format)

It already was when I view the file.

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 83 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should have the open brace on a separate row.

Done.

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 87 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we align the equal signs (as the other examples)? (If using VSCode, hit Alt+Shift+F to auto-format)

It already was when I view the file. Auto-format won't work in my case because I don't have formatting set like you do for this project.

---

Examples/Resources/SqlAGDatabase/1-AddDatabaseToAvailabilityGroup.ps1, line 79 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

The property does not exist as SqlInstanceName it shows up in documentation as InstanceName. So I am not aware of why the test are failing to see it properly.

---

Examples/Resources/SqlAGDatabase/3-MatchDefinedDatabaseInAvailabilityGroup.ps1, line 74 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

The property does not exist as SqlInstanceName it shows up in documentation as InstanceName. So I am not aware of why the test are failing to see it properly.

---

Examples/Resources/SqlAGReplica/1-CreateAvailabilityGroupReplica.ps1, line 80 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

InstanceName

Think this should be SQLInstanceName, but I'm in favor of replacing SQLInstanceName to InstanceName throughout this example. But you decide which way you want to go. 🙂

The property does not exist as SqlInstanceName it shows up in documentation as InstanceName. So I am not aware of why the test are failing to see it properly.

[wsmelton]

Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @johlju and @wsmelton)

---

CHANGELOG.md, line 14 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think this lines fails in the tests.
https://ci.appveyor.com/project/PowerShell/sqlserverdsc/build/9.0.424.0?fullLog=true#L2133

Fixed character length of line.

[johlju]

Reviewed 1 of 1 files at r2, 2 of 5 files at r3.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 87 at r1 (raw file):

Previously, wsmelton (Shawn Melton) wrote…

It already was when I view the file. Auto-format won't work in my case because I don't have formatting set like you do for this project.

Looks good. FYI. This project has a workspace setting file for VSCode, so it should override?

---

Examples/Resources/SqlAGDatabase/1-AddDatabaseToAvailabilityGroup.ps1, line 79 at r1 (raw file):

Previously, wsmelton (Shawn Melton) wrote…

The property does not exist as SqlInstanceName it shows up in documentation as InstanceName. So I am not aware of why the test are failing to see it properly.

Ah, my bad! Should have been more clear, didn't thought of that the parameter was named equal. :/

The row

 InstanceName         = $Node.InstanceName

SHould be

 InstanceName         = $Node.SqlInstanceName

$Node.SqlInstanceName come from the the ConfigurationData block at the top of the file. I'm in favor of changing the SqlInstanceName in the configuration block to InstanceName, and replacing $Node.SqlInstanceName to $Node.InstanceName. But it's good either way you do it, so please you choose what name it should be. 🙂

Same for the other example files.

[wsmelton]

Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @johlju and @wsmelton)

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 87 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Looks good. FYI. This project has a workspace setting file for VSCode, so it should override?

That is a bug in VS Code because I have other settings that workspace settings will not overwrite user settings. The default for VS Code is to have brace on same line, which is also what I explicitly set in my user settings. The workspace settings for this project are not being applied when I open it.

---

Examples/Resources/SqlAGDatabase/1-AddDatabaseToAvailabilityGroup.ps1, line 79 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Ah, my bad! Should have been more clear, didn't thought of that the parameter was named equal. :/

The row

 InstanceName         = $Node.InstanceName

SHould be

 InstanceName         = $Node.SqlInstanceName

$Node.SqlInstanceName come from the the ConfigurationData block at the top of the file. I'm in favor of changing the SqlInstanceName in the configuration block to InstanceName, and replacing $Node.SqlInstanceName to $Node.InstanceName. But it's good either way you do it, so please you choose what name it should be. 🙂

Same for the other example files.

I opted to go ahead and update the property name in the configuration data because it is set to InstanceName in some of the other examples.

If no one ever gets to it I will come back around and update the examples to be consistent on that property.

[johlju]

Almost there, we need a rebase of this PR since I merged other PR's.

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wsmelton)

a discussion (no related file):
There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

---

---

Examples/Resources/SqlAG/3-CreateAvailabilityGroupDetailed.ps1, line 87 at r1 (raw file):

Previously, wsmelton (Shawn Melton) wrote…

That is a bug in VS Code because I have other settings that workspace settings will not overwrite user settings. The default for VS Code is to have brace on same line, which is also what I explicitly set in my user settings. The workspace settings for this project are not being applied when I open it.

Aha didn't know that! Good to know.

[wsmelton]

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

There are merge conflicts since your PR was not based on the latest changes. Could you please rebase against branch dev using git rebase (not by using git pull or git merge, to keep the commit history). If you don't know how to rebase your local dev and working branch, please look at how to Resolve merge conflicts.
Let me know if you need any assistance.

Ha, that documentation is backwards to how Git is documented on remotes (e.g. origin should be my fork and not the upstream remote, this goes against every example in the documentation). I'll have to read through that a few times to make sure I don't break my branch.

---

[johlju]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johlju)

a discussion (no related file):

Previously, wsmelton (Shawn Melton) wrote…

Ha, that documentation is backwards to how Git is documented on remotes (e.g. origin should be my fork and not the upstream remote, this goes against every example in the documentation). I'll have to read through that a few times to make sure I don't break my branch.

I think it depends on what you clone with (and how you clone), using git clone https://github.com/<account>/<repo> automatically use the 'origin' name for the 'upstream' remote.
You can always copy the repo folder to backup location so you can start over. :) Let me know if you have any problems, happy to help.

---

[wsmelton]

Rebased, think I did it properly. Conflict took a bit to clear but think I got it in there right.

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju and @wsmelton)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

I think it depends on what you clone with (and how you clone), using git clone https://github.com/<account>/<repo> automatically use the 'origin' name for the 'upstream' remote.
You can always copy the repo folder to backup location so you can start over. :) Let me know if you have any problems, happy to help.

Being that we have to fork the repository to our own accounts then our origin is going to be my fork wsmelton/sqlserverdsc and I then add powershell/sqlserverdsc as the upstream. The way I read the documents I would clone PowerShell/sqlserverdsc and that becomes my origin, but it won't work that way directly because I can't publish branches to that repository.

In the docs you have it listed as the following:

my	https://github.com/vors/xActiveDirectory (fetch)
my	https://github.com/vors/xActiveDirectory (push)
origin	https://github.com/PowerShell/xActiveDirectory (fetch)
origin	https://github.com/PowerShell/xActiveDirectory (push)

It won't turn out like this when you use GitHub to form the repository and then use VS Code to clone your fork to your local machine. I understand Git and how GitHub works but just the way all that is laid out a beginner to git would get a bit confused. (I'm only aware of this because I deal with it on OSS project I maintain.)

---

[johlju]

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju)

a discussion (no related file):
The docs are written as a beginner followed the previous sections in the Getting Started with GitHub docs. There is a note about the remotes should point correct. Then the remotes would be correct.

Note: These steps require that you have added the remote as described above. Run git remote -v to verify that you have the remotes my pointing to your fork repository and origin pointing to the original repository.
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#clone-repository-from-github

I don't think it is possible to write the docs (and keep it updated) for every scenario and every tool (they are written to use git). But we could add a text to the note above that one should change to the appropriate remote name. I will fix that.

Using git this way, gives the remote

git clone https://github.com/PowerShell/SqlServerDsc
cd SqlServerDsc
git remote add my https://github.com/johlju/SqlServerDsc

After that it is possible to choose what remote to push to in VS Code. I can't seem to find any way to add a remote from within VS Code after cloning in VS Code. I personally only use VS Code to commit, everything else I do from command line, so never realized that was missing (or there is a trick to it that I can't find).

---

[johlju]

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

The docs are written as a beginner followed the previous sections in the Getting Started with GitHub docs. There is a note about the remotes should point correct. Then the remotes would be correct.

Note: These steps require that you have added the remote as described above. Run git remote -v to verify that you have the remotes my pointing to your fork repository and origin pointing to the original repository.
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#clone-repository-from-github

I don't think it is possible to write the docs (and keep it updated) for every scenario and every tool (they are written to use git). But we could add a text to the note above that one should change to the appropriate remote name. I will fix that.

Using git this way, gives the remote

git clone https://github.com/PowerShell/SqlServerDsc
cd SqlServerDsc
git remote add my https://github.com/johlju/SqlServerDsc

After that it is possible to choose what remote to push to in VS Code. I can't seem to find any way to add a remote from within VS Code after cloning in VS Code. I personally only use VS Code to commit, everything else I do from command line, so never realized that was missing (or there is a trick to it that I can't find).

Added a change to the "note" here: PowerShell/DscResources#441

It seems the rebase didn't work. :/ We got a lot of commits in the PR now that are already part of the dev branch
https://github.com/PowerShell/SqlServerDsc/pull/1208/commits

Could you try it again? If you list the remote names you got and where they are pointing (git remote -v) I could assist getting the commands.

---

[wsmelton]

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju and @wsmelton)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Added a change to the "note" here: PowerShell/DscResources#441

It seems the rebase didn't work. :/ We got a lot of commits in the PR now that are already part of the dev branch
https://github.com/PowerShell/SqlServerDsc/pull/1208/commits

Could you try it again? If you list the remote names you got and where they are pointing (git remote -v) I could assist getting the commands.

origin  https://github.com/wsmelton/sqlserverdsc.git (fetch)
origin  https://github.com/wsmelton/sqlserverdsc.git (push)
upstream        https://github.com/powershell/sqlserverdsc.git (fetch)
upstream        https://github.com/powershell/sqlserverdsc.git (push)```

---

[johlju]

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju)

a discussion (no related file):

Previously, wsmelton (Shawn Melton) wrote…

origin  https://github.com/wsmelton/sqlserverdsc.git (fetch)
origin  https://github.com/wsmelton/sqlserverdsc.git (push)
upstream        https://github.com/powershell/sqlserverdsc.git (fetch)
upstream        https://github.com/powershell/sqlserverdsc.git (push)```
</blockquote></details>

These are the commands I would have run.

```powershell
git checkout dev
git fetch upstream dev
git rebase upstream/dev
git push origin dev --force
git checkout createag-example-updates
git rebase origin/dev
# fix any merge conflicts, run `git rebase --continue` as necessary 
# When there are no more conflicts. continue with the next row.
git push origin createag-example-updates --force

---

[johlju]

The comment from review got strange, commenting here instead.

These are the commands I would have run.

git checkout dev
git fetch upstream dev
git rebase upstream/dev
git push origin dev --force
git checkout createag-example-updates
git rebase origin/dev
# fix any merge conflicts, run `git rebase --continue` as necessary 
# When there are no more conflicts. continue with the next row.
git push origin createag-example-updates --force

[johlju]

At any time during rebase, while there is a conflict, you can abort by using git rebase --abort.

Also, if for some reason there are nothing to commit when you fix a merge conflict, it can happen if the files are changed to exactly what are already being tracked (is committed), you can use git rebase --skip. But only do that if there are nothing to stage for commit.
The last could happen if there was a problem with a rebase prior, and commits was added manually. It should normally not happen when the rebase is correct the first time.

[wsmelton]

I ran git fetch upstream git rebase upstream/dev

This is equivalent to what you just did I just don't update my local branches of dev and work directly from my createag-example-updates branch...which is the same thing. Rebasing against the upstream directly should have no difference.

It was likely how I handled the conflict. Which just a note since your guidelines require updating the changelog that file will always be a conflict for users to deal with being that there are a number of PRs out there waiting to be merged.

I will go reset my branch and see if I can get the rebase in properly.

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @johlju and @wsmelton)

[wsmelton]

You will have to just close this PR as the branch is messed up now and I can't get it to revert cleanly. I'll come back around and do a new PR at some point.

[johlju]

@wsmelton I rebeased it for you, hope you don't mind. Love to get this PR through. 🙂

[johlju]

@wsmelton Missed the question you asked in #1208 (comment).

I personally think Reviewable is far superior to GitHub review, and gives you so much more control over the conversations. DSC Resource Kit is using Reviewable, not sure other repositories does. Just last week GitHub added that you can resolve review comments, so they slowly getting better.

The 80 char limit in markdown makes it easier to read the markdown when reviewing the raw files.

[johlju]

Reviewed 4 of 4 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[wsmelton]

How did you get it to reset?

[johlju]

I just ran the commands I posted above, except not pushing dev to your fork since I don't have permission. The rebase replayed your commits over the updated dev branch.

[johlju]

Although. Some commit did not want to stage any changes, so I skipped those commits (the changes was already correct by a previous commit).

[johlju]

Just waiting for the tests to pass, then I merge this.

[johlju]

Since there was a release yesterday, the branch was not up to date again, and the change log needed to be updated, since git otherwise merging it into the wrong location.

Reviewed 4 of 4 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

@wsmelton this is merged now! Thanks for you help fixing this! 😃