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

SqlServerDatabaseMail: New resource for configuring Database Mail #145

Merged
merged 34 commits into from
Jan 4, 2018
Merged

SqlServerDatabaseMail: New resource for configuring Database Mail #145

merged 34 commits into from
Jan 4, 2018

Conversation

SQLHorizons
Copy link
Contributor

@SQLHorizons SQLHorizons commented Oct 4, 2016

Pull Request (PR) description

  • Changes to SqlServerDsc
    • Added new resource SqlServerDatabaseMail for configuring SQL Server
      database mail.

This Pull Request (PR) fixes the following issues:
Fixes #155

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

@msftclas
Copy link

msftclas commented Oct 4, 2016

Hi @SQLHorizons, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

msftclas commented Oct 4, 2016

@SQLHorizons, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@mbreakey3 mbreakey3 added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Oct 10, 2016
@johlju
Copy link
Member

johlju commented Oct 16, 2016

@SQLHorizons You need to resolve the check that failed to be able to get a review on this PR. See the failed test here https://ci.appveyor.com/project/PowerShell/xsqlserver/build/2.0.369.0
Looks like you have the wrong encoding on the module file, if you change the file encoding to UTF-8 I think the test will be happy.

Could you also please change the name of the resource to something that starts with 'MSFT_xSQLServer', like 'MSFT_xSQLServerDatabaseMail'. Only place where the prefix 'MSFT_' should not be used in in the friendly name in the schema.mof file (see other resources for examples).

@johlju
Copy link
Member

johlju commented Oct 16, 2016

@SQLHorizons Also make sure all files end with a blank line.

@SQLHorizons
Copy link
Contributor Author

@johlju thank you for your input, made the changes, but still got some work on this as its only good for creating the account from scratch. I know what I need to do, just got to find the time.

@johlju
Copy link
Member

johlju commented Oct 17, 2016

@SQLHorizons It's good work so far! Hope you get the time to finish this soon, I know what you mean about finding time. Let us know if you need any help! We are here for each other! 😄

Some suggestions after a quick glance of your code

You can find all the guidelines here: https://github.com/PowerShell/DscResources

Update: Wrong URL to the Unit template.

@kwirkykat
Copy link
Contributor

@SQLHorizons @johlju I agree this resource needs to be renamed. I like @johlju's suggestion of 'MSFT_xSQLServerDatabaseMail'.

@SQLHorizons
Copy link
Contributor Author

@johlju, @kwirkykat Agreed will see what I can do over the next week, and yes will go with the name suggestion.

@johlju
Copy link
Member

johlju commented Nov 7, 2016

@SQLHorizons don't want to rush you in anyway. Just curious if you had gotten some time to start look at this. Let us know if you need any help. 😄

@SQLHorizons
Copy link
Contributor Author

@johlju sorry been busy with a lot of other stuff lately, am I holding something up?

@johlju
Copy link
Member

johlju commented Nov 13, 2016

No, you are not holding anything up. Was just checking status :) And I'm looking forward getting this resource into the module :)

@johlju
Copy link
Member

johlju commented Jun 21, 2017

@SQLHorizons Do you have time to finish this one? If you feel that you don't have time, then please tick 'Allow edits from maintainers' to the right here, around Notifications. Then I can help you get this finished.

In the meantime I label this one as abandoned.

@johlju johlju added the abandoned The pull request has been abandoned. label Jun 21, 2017
@SQLHorizons
Copy link
Contributor Author

@johlju, sorry you're probably correct. the 'Allow edits from maintainers' was already ticked though.

@johlju
Copy link
Member

johlju commented Jun 22, 2017

@SQLHorizons Great that it was ticked. I can help an see if I can help get this PR thru.

Side note: I can't see if they are ticked, I raised an issue to GitHub and asked for them to add the checkbox, grayed out, for all (or at least maintainers), so that I can see the status and don't have to ask every time. 😄

@johlju johlju changed the title Created xdBmail resource for deploying db mail on new SQL build xSQLServerDatabaseMail: New resource for deploying database mail Jun 22, 2017
@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #145 into dev will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #145    +/-   ##
====================================
- Coverage    97%    96%    -1%     
====================================
  Files        31     33     +2     
  Lines      3673   4017   +344     
====================================
+ Hits       3581   3895   +314     
- Misses       92    122    +30

@johlju
Copy link
Member

johlju commented Sep 26, 2017

Closing an reopening to get correct CLA status.

@johlju johlju closed this Sep 26, 2017
@johlju johlju reopened this Sep 26, 2017
@vors vors removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Sep 26, 2017
@johlju johlju added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Sep 26, 2017
@johlju johlju removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Dec 7, 2017
@johlju
Copy link
Member

johlju commented Dec 25, 2017

Renamed this resource to align with new naming convention - please suggest other name if someone thinks SqlServerDatabaseMail is not appropriate.

I will try to work on this in the next couple of weeks so we can get this one to the finish line since it been in PR for over a year. 😄 @SQLHorizons was kind enought to tick the 'Allow edits from maintainers', so I can continue the work in the same PR.
@SQLHorizons if you feel taking over the work at any point, just let me know and I'm happy to hand you the reins.

@johlju
Copy link
Member

johlju commented Jan 4, 2018

Reviewed 1 of 6 files at r3, 1 of 4 files at r6, 4 of 4 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju
Copy link
Member

johlju commented Jan 4, 2018

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@johlju johlju merged commit e45d6c1 into dsccommunity:dev Jan 4, 2018
@johlju
Copy link
Member

johlju commented Jan 4, 2018

@SQLHorizons took a while, but now finally the work you started has been merged! Thanks for your work! 😄

@johlju johlju removed the needs review The pull request needs a code review. label Mar 12, 2018
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.

SqlServerDatabaseMail: New resource
8 participants