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

Fixes issues 3, 4 and 7 #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixes issues 3, 4 and 7 #9

wants to merge 4 commits into from

Conversation

bugal
Copy link

@bugal bugal commented Feb 4, 2019

Fixes some issues, so it works on clustered scenarios, and also adds some informational outputs when it can't find SQL Server instances or gets connection errors.
Tested on SQL Server 2008R2, SQL Server 2012 and SQL Server 2012 Cluster.

@aph3rson
Copy link

@bugal thanks for the PR - just a couple quick notes I was going to mention:

  • It looks like your commits don't have an email set, so you wouldn't get credit for commits to the repo. If you'd like to go back and reauthor your commits (this guide should be helpful), it should resolve that.
  • Your commit titles have #iss3/etc. in them - this won't link to the specific issue you're referring to. Instead, you should add something along the lines of Fixes NetSPI/Powershell-Modules#3 or Works on NetSPI/Powershell-Modules#3, which Github will autolink to that issue, and you'd see a handy note there accordingly.

Note that both of these changes are considered "rewriting history" on Github - therefore, a force push is required, which generally isn't kosher on PRs. However, if you'd like to do a force-push on your branch with these changes, I can get this assigned for review.

@bugal
Copy link
Author

bugal commented Apr 11, 2019

Thanks for the reply, @aph3rson, I'm still getting used to github.
I did as you oriented, and it appears to be ok now.

@aph3rson
Copy link

I've taken a precursory look at the code, and it looks good. I'm asking the reporters of #4 to try the changes out on their side, as I don't have a clustered environment handy.

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